[PE-193] feat: flat lists #6316
Conversation
WalkthroughThe pull request introduces a comprehensive enhancement to the list functionality in the Plane editor. It adds a new flat list extension with advanced features like indentation, dedenting, splitting, and various list types (bullet, ordered, task, and toggle). The changes span multiple files, introducing new commands, utilities, and plugins to manage list interactions more effectively. The implementation focuses on improving the user experience by providing more intuitive list management and better handling of list-related operations. Changes
Sequence DiagramsequenceDiagram
participant User
participant Editor
participant ListExtension
participant ListCommands
User->>Editor: Interact with list
Editor->>ListExtension: Detect list interaction
ListExtension->>ListCommands: Trigger appropriate command
alt Indent List
ListCommands->>Editor: Increase list item depth
else Dedent List
ListCommands->>Editor: Decrease list item depth
else Toggle List Type
ListCommands->>Editor: Change list type
else Split List
ListCommands->>Editor: Split current list item
end
ListExtension->>Editor: Update document state
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (83)
packages/editor/src/core/extensions/flat-list/core/utils/is-node-selection.ts (1)
3-5: Consider a more robust implementation for type checking.While the current implementation works, it could be made more robust to prevent false positives. The
NodeSelectiontype from ProseMirror has additional properties that could be checked.Consider this alternative implementation that performs more thorough type checking:
-export function isNodeSelection(selection: Selection): selection is NodeSelection { - return Boolean((selection as NodeSelection).node); +export function isNodeSelection(selection: Selection): selection is NodeSelection { + if (!selection) return false; + // Check multiple properties that are specific to NodeSelection + return ( + 'node' in selection && + 'from' in selection && + 'to' in selection && + selection.from === selection.to - 1 + ); +}This implementation:
- Safely handles null/undefined inputs
- Uses
inoperator for type narrowing instead of type assertion- Checks additional properties specific to NodeSelection
- Validates the selection range (NodeSelection always spans exactly one node)
packages/editor/src/core/extensions/table/table/table.ts (1)
119-119: Remove debug console statement.
Leaving aconsole.login production code can clutter logs and potentially leak implementation details. Recommend removing or using a more controlled logging approach if needed for debugging.- console.log("ran");packages/editor/src/core/extensions/flat-list/core/schema/parse-dom.ts (3)
14-26: Minor duplication betweendiv[data-list-kind]anddiv[data-list].
Both rules exhibit near-identical logic for extracting attributes. You could unify them if you want to minimize repetition. For example, factor out a helper function to extract these attributes, then call it in each rule.+function extractListAttributes(element: Element, defaultKind: ListKind = "bullet"): ListAttributes { + return { + kind: (element.getAttribute("data-list-kind") || defaultKind) as ListKind, + order: parseInteger(element.getAttribute("data-list-order")), + checked: element.hasAttribute("data-list-checked"), + collapsed: element.hasAttribute("data-list-collapsed"), + }; +} /* Then in the existing rules: */ -{ - tag: "div[data-list-kind]", - getAttrs: (element): ListAttributes => { - ... - return { ... }; - }, -}, +{ + tag: "div[data-list-kind]", + getAttrs: (element): ListAttributes => + typeof element === "string" ? {} : extractListAttributes(element), +},
44-93: Cautious iteration over child nodes.
The loop at lines 49–54 checks up to three nested child nodes before deciding if we have a checkbox input. This can be brittle if an unexpected DOM structure or additional nesting is introduced. Consider a more robust traversal or a single function that safely finds the<input>element.-for (let i = 0; i < 3 && checkbox; i++) { - if (["INPUT", "UL", "OL", "LI"].includes(checkbox.nodeName)) { - break; - } - checkbox = checkbox.firstChild as HTMLElement | null; -} +function findCheckboxInput(root: HTMLElement | null, maxDepth = 3): HTMLInputElement | null { + let current = root; + let depth = 0; + while (current && depth < maxDepth) { + if (current.nodeName === "INPUT" && current.getAttribute("type") === "checkbox") { + return current as HTMLInputElement; + } + if (["UL", "OL", "LI"].includes(current.nodeName)) { + break; + } + current = current.firstChild as HTMLElement | null; + depth++; + } + return null; +}
77-86: Regex-based task detection.
Stripping the"[x]"or"[ ]"tokens from text content is convenient, but it could misfire if text is localized or if the bracket pattern appears incidentally. If your use case expands or if i18n is introduced, consider a more structured approach to identify tasks.packages/editor/src/core/extensions/flat-list/core/utils/range-to-string.ts (1)
10-13: Consider adding boundary checks forstartIndexandendIndex.
If the provided indices exceed the content bounds, this function might generate unexpected results or errors. As a good-to-have, add checks to fail gracefully or return an empty string if the range is bad.Here's a sample snippet that returns an empty string if the indices are out of range:
export function rangeToString(range: NodeRange): string { const { parent, startIndex, endIndex } = range; + if (startIndex < 0 || endIndex > parent.content.size || startIndex >= endIndex) { + return ""; + } return cutByIndex(parent.content, startIndex, endIndex).toString(); }packages/editor/src/core/extensions/flat-list/core/utils/auto-fix-list.ts (2)
69-79: Clarify theindex === 1condition.
It’s not immediately obvious why splitting is targeted only whenindex === 1. Consider documenting the rationale behind this condition.
81-101: Consider multiple passes for comprehensive fixes.
In complex documents, a single pass of joins and splits may not fix all list inconsistencies. You could repeatedly callfixListuntil no further changes apply or combine it into one iterative loop.packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts (1)
61-61: Rewrite assignment in loop condition for clarity.
This code snippet uses assignment within thewhilecondition, which can be confusing. Refactor the pattern to avoid multiple side effects in a single expression.-while (((next = child.nextElementSibling), next?.tagName === child.tagName)) { - child.append(...Array.from(next.children)); - next.remove(); -} +let next = child.nextElementSibling; +while (next && next.tagName === child.tagName) { + child.append(...Array.from(next.children)); + next.remove(); + next = child.nextElementSibling; +}🧰 Tools
🪛 Biome (1.9.4)
[error] 61-61: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/editor/src/core/extensions/flat-list/core/commands/unwrap-list.ts (1)
70-93: Optional fallback for missingattrs.
InisTargetListandisTargetListsRange, the code checks(node.attrs as ListAttributes). Ifattrsis unexpectedly undefined, type coercion might fail. Adding a default or a safe check can make the code more robust.- return (node.attrs as ListAttributes).kind === kind + return (node.attrs as ListAttributes)?.kind === kindpackages/editor/src/core/extensions/flat-list/core/migrate.ts (1)
3-44: Consider logging or handling unrecognized list types.
Currently, only known list types (bullet_list,ordered_list,task_list, etc.) are explicitly handled. Any unknown or custom list types are simply passed through. Logging or handling them might make migrations more transparent.packages/editor/src/core/extensions/flat-list/core/commands/wrap-in-list.ts (3)
30-36: Check for invalid block ranges.The early return on lines 34-36 is a good safeguard if no valid block range is found. Consider adding an inline comment to clarify why we bail out at this point, especially for future maintainers unfamiliar with ProseMirror’s range mechanics.
57-63: Optimize multiple passes.When iterating from the end index downward, each child is handled individually (list or non-list). For large selections, consider grouping consecutive nodes in a single pass to minimize transformations and possibly enhance performance.
93-101: Early exit for inline checks.
rangeAllowInlineContentscans all child nodes in the range. If performance becomes a concern with very large blocks, consider returning promptly upon finding the first node with inline content.packages/editor/src/core/extensions/flat-list/heading-list-extension.ts (1)
50-97: Graceful error handling in commands.Wrapping the creation logic in a
try/catchwithincreateHeadedListis helpful. Consider providing user-facing feedback (e.g., toast notifications) rather than only logging to the console, especially if errors might be triggered by user actions within an editor UI.packages/editor/src/core/extensions/flat-list/core/commands/indent-list.ts (2)
105-130: Check the split index boundaries.
splitPosis derived fromsplitIndexat line 112. Particularly in edge cases (e.g.,splitIndexis 0 or equal to endIndex), confirm you’re not accidentally referencing invalid positions when creatingrange1orrange2.Shall I write a test snippet for boundary conditions to verify correctness?
135-185: Consider a fallback if merging fails.When moving the selected nodes into the previous list node fails or is invalid (e.g., mismatched node types, or the parent is locked), the code returns
false. Consider adding a fallback approach to wrap or re-insert the nodes to maintain a valid document structure. This can help avoid dropping user content inadvertently.packages/editor/src/core/extensions/extensions.tsx (4)
60-62: Review disabled StarterKit list features.
bulletList,orderedList, andlistItemare set tofalse. Ensure that all commands reliant on these default configurations are replaced or supplemented by the newly added list extensions (BulletList, OrderedList, ListItem, and FlatListExtension).
85-93: Remove redundant input rules if not using them.
BulletList.extend(...)has an emptyaddInputRules()method returning no rules. If these additions are unneeded, consider removing or documenting their future purpose.
94-102: Remove redundant input rules in OrderedList.Similar to BulletList, the
addInputRules()method is returning an empty array. Consider removing or replacing placeholders with meaningful list-related rules.
112-120: Check TaskList’s space-y-2 spacing.
TaskListis configured with specificHTMLAttributes. Ensure consistent spacing with other list types and confirm that tasks remain distinctly styled from bullet/ordered lists.packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts (4)
5-16: Use consistent naming for utility imports.
withAutoFixListand the block-boundary utilities are introduced here. Confirm these match naming conventions in other modules likesplit-list.tsto avoid confusion.
22-36: Clarify usage of DedentListOptions.
from?: numberandto?: numberare optional. Document the typical scenario for dedenting partial ranges vs. entire selection to help future maintainers.
150-157: Review safeLiftRange pre-processing.
moveRangeSiblingsmodifies the transaction, then updates$fromand$to. Check for race conditions if other code modifies the transaction concurrently.
159-209: Move sibling logic correctness.The code merges trailing siblings into the last child. Ensure that empty or partially empty nodes aren’t accidentally merged.
packages/editor/src/core/hooks/use-editor.ts (1)
116-145: Avoid losing user selection mid-migration.Your code attempts to preserve
savedSelection. Edge cases (e.g., selection outside doc size) might cause unexpected scroll or selection states. Consider fallback logic if the old selection is invalid.packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts (3)
239-239: Use optional chaining for safer property access.Per static analysis, you can simplify
node && node.type.spec.disableDropCursorusing optional chaining.- const disableDropCursor = node && node.type.spec.disableDropCursor; + const disableDropCursor = node?.type?.spec?.disableDropCursor;🧰 Tools
🪛 Biome (1.9.4)
[error] 239-239: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
254-254: Use optional chaining forthis.editorView.dragging?.slice.Eliminates the double check
if (this.editorView.dragging && this.editorView.dragging.slice).- if (this.editorView.dragging && this.editorView.dragging.slice) { + if (this.editorView.dragging?.slice) {🧰 Tools
🪛 Biome (1.9.4)
[error] 254-254: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
106-113: Document the disableDropCursor NodeSpec feature.Developers may overlook or misuse the
disableDropCursorproperty. Provide inline doc comments or examples on how to override it for specific nodes.packages/editor/src/core/extensions/drop-cursor-working.ts (4)
47-48: Consider removing debug logs.You have
console.log("aaya")in production code. To avoid cluttering the console and potentially revealing internal implementation details, consider removing or wrapping it with a debug flag or logging utility.
119-119: Avoid usingthis as anyif possible.Casting
thistoanycan bypass type safety and lead to runtime issues. Consider giving the class a clear type for the event handlers to avoid losing type information.
222-222: Use optional chaining for clarity.Static analysis suggested using an optional chain. If
listElementmight be null, consider usinglistElement?.nextElementSiblingor an equivalent pattern for more concise and robust code.🧰 Tools
🪛 Biome (1.9.4)
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
234-239: Remove leftover debug logs.The
console.log("asfd")andconsole.log("asdff")calls seem to be leftover debug logs. Removing them or using a proper logger is preferable in production code.packages/editor/src/core/extensions/drop-cursor-weird.ts (4)
48-48: Remove console log in production code.The
console.log("aaya")statement might unintentionally leak debug information in production. Replacing it with a logging utility or removing it can help keep the console clean.
54-74: Consider more robust error handling for drop events.Currently, if
posis missing or invalid, you simply returnfalse. Consider providing fallback logic or error handling for exceptional cases to prevent partial or inconsistent updates to the editor state.
212-212: Optional chaining suggestion.Static analysis recommends using optional chaining for safer property access in
node?.type?.spec.disableDropCursor. This reduces repetitive null checks and ensures consistent behavior ifnodeis null or undefined.🧰 Tools
🪛 Biome (1.9.4)
[error] 212-212: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
226-226: Evaluate optional chaining usage here.As with line 212, consider
this.editorView.dragging?.sliceto shorten your logic and handle undefineddraggingstates gracefully.🧰 Tools
🪛 Biome (1.9.4)
[error] 226-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-huh.ts (3)
48-48: Avoid production console logging.Using
console.log("asdff")in production can clutter the logs. Remove or wrap this statement for debugging only.
58-58: Remove extraneous debug statements.The
console.log("asdff")line is similarly redundant. Prefer a logging framework or remove debug output for production readiness.
265-295: Check optional chain feasibility.Static analysis suggests optional chaining for lines like
this.editorView.dragging?.slice. Confirm that references are resilient to null or undefined to avoid potential runtime errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: expected
,but instead found:Remove :
(parse)
[error] 268-268: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 273-273: expected
,but instead found:Remove :
(parse)
[error] 273-273: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 281-281: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 268-271: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
packages/editor/src/core/extensions/drop-cursor.ts (2)
266-266: Consider optional chaining.For improved readability and safety, using optional chaining can make the code more concise where
nodeornode.typemight be null. For example,node?.type?.spec.disableDropCursor.🧰 Tools
🪛 Biome (1.9.4)
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
272-272: Optional chaining forthis.editorView.dragging.Consistently apply optional chaining to
this.editorView.dragging?.sliceto avoid potential null dereferences and reduce nested conditionals.🧰 Tools
🪛 Biome (1.9.4)
[error] 272-272: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor.optimized.ts (2)
271-271: Optional chaining in property access.Use
node?.type?.spec.disableDropCursorto handle the case wherenodeornode.typemight be undefined. This prevents possible runtime errors.🧰 Tools
🪛 Biome (1.9.4)
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
277-277: Optional chaining onthis.editorView.dragging?.slice.Following the same rationale, optional chaining can help clarify null vs. defined states without redundant branching logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 277-277: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/flat-list/core/utils/cut-by-index.ts (1)
1-6: Caution with internal API usage.
Relying onfragment.cutByIndexmay break if it changes in future Tiptap releases. Keep an eye on release notes or consider an alternative approach if available.packages/editor/src/core/extensions/flat-list/core/utils/patch-command.ts (1)
5-7: Guard against no-op patches.If
patchmakes no changes to the transaction, the flow still re-dispatches the original transaction. This is correct behavior, but it might be helpful to explicitly check whether any meaningful updates were made, especially if performance or clarity is a concern.packages/editor/src/core/extensions/flat-list/core/commands/enter-without-lift.ts (1)
1-10: Consider adding an optionalliftEmptyBlockstep.Currently,
liftEmptyBlockis omitted by design. If there's a scenario where partial or conditional lifting is beneficial, consider implementing a custom command or an optional parameter. This ensures flexibility without straying from the core vision of “enter without lift.”packages/editor/src/core/extensions/flat-list/core/utils/map-pos.ts (1)
3-16: Avoid mutating theposvariable externally to the closure.
mapPosmutatesposwithin thegetPosfunction, which is fine for an enclosed closure. However, keep in mind that multiple calls togetPosre-map the samepos. If you need independent position tracking, clone or store each position in a separate variable to prevent unintended side effects.packages/editor/src/core/extensions/flat-list/core/utils/browser.ts (2)
4-4: Optional chaining recommendation.To enhance clarity and reduce logical complexity, consider using optional chaining and nullish coalescing:
-const agent = (nav && nav.userAgent) || ""; +const agent = nav?.userAgent ?? "";🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
12-12: Straightforward Safari detection.The check accurately identifies Safari by vendor while excluding IE/Edge. Keep in mind that browser detection can be fragile over time; consider feature detection where feasible.
packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-end.ts (1)
6-11: Implementation logic is sound.Like
atTextblockStart, this function correctly checks if the cursor is at the end of the text block. Consider adding tests for boundary conditions (e.g., empty paragraphs or multi-node paragraphs) to ensure robust coverage.packages/editor/src/core/extensions/flat-list/core/types.ts (1)
1-2: Consider narrowingkindinListAttributestoListKind.
Currently,ListAttributesdefines thekindproperty as a genericstring, whereasListKindis a union of specific string literals. Aligning them would help prevent inconsistencies between supported list kinds and assigned attribute values.-export interface ListAttributes { - kind?: string; +export interface ListAttributes { + kind?: ListKind;packages/editor/src/core/extensions/flat-list/core/utils/safe-lift.ts (1)
14-20: Comprehensive coverage of lifting operations.
safeLiftFromToensures only valid block ranges are lifted. Consider expanding error handling or logging to capture scenarios where a lift is not possible.packages/editor/src/core/extensions/flat-list/core/plugins/index.ts (1)
9-29: Comprehensive plugin array creation.
ThecreateListPluginsfunction neatly aggregates related plugins, making it easy to extend or selectively include them. The JSDoc is informative.Consider providing a config object letting consumers selectively enable or disable specific plugins.
packages/editor/src/core/extensions/flat-list/core/commands/protect-collapsed.ts (1)
3-3: Consider caching repeated checks.
isCollapsedListNodemay be invoked repeatedly for each node withinnodesBetween. Though this is likely negligible, you could cache or otherwise reduce repeated calls if performance becomes critical.packages/editor/src/core/extensions/flat-list/core/commands/join-textblocks-around.ts (1)
24-33: Improve clarity of step size check.
step.slice.size >= afterPos - beforePoscan be more readable if separated into simpler or named checks to clarify the purpose of the comparison.packages/editor/src/core/extensions/flat-list/core/node-view.ts (1)
27-34: Update logic might skip subtle style changes.
Theupdatefunction checkssameMarkup, but if the node has changed style-based attributes that do not affect markup directly, you may need a more granular check to handle them.packages/editor/src/core/extensions/flat-list/core/schema/node-spec.ts (1)
20-24: Clarify use of group naming.
flatListGroupis descriptive but ensure it doesn't conflict with other list groups or naming inside the schema.packages/editor/src/core/extensions/flat-list/core/commands/toggle-collapsed.ts (1)
30-53: Consider clarifying toggle logic in comments.
While the command usage is mostly self-explanatory, it could be beneficial to include a brief inline comment or docstring highlighting how thecollapsedparameter overrides the toggling behavior.+/** + * If `collapsed` is provided, it sets that value. Otherwise, it toggles + * the current `collapsed` attribute. + */ export function createToggleCollapsedCommand({ collapsed = undefined, ...packages/editor/src/core/extensions/flat-list/core/dom-events.ts (2)
10-30: Good pattern for custom event handling.
Preventing default behavior and translating the event to a ProseMirror position is a solid approach. Watch for potential offset inaccuracies(-10, -10)across different browsers.-const pos = view.posAtDOM(target, -10, -10); +// Consider verifying the values or making them configurable +const pos = view.posAtDOM(target, -10, -10);
32-50: Refactor the early-return path for clarity.
Consider simplifying the block that checksif (!isListNode(list)) return false;for readability, especially if more conditions are added later.packages/editor/src/core/extensions/flat-list/core/commands/set-safe-selection.ts (1)
53-70:setVisibleSelectionensures user clarity.
Expanding the collapsed node on selection is a user-friendly approach. Consider if this action should be optional or dependent on user preference.packages/editor/src/core/extensions/flat-list/core/index.ts (2)
10-19: Consider adding consistent naming patternsFor clarity and discoverability, ensure that your command factory functions follow consistent naming conventions (e.g.,
createSomethingCommand). While it is consistent in many places, consider confirming that all newly introduced commands strictly follow this pattern to help new contributors find them quickly.
20-38: Comprehensive list utility exportsExporting these plugs and event handlers (e.g.,
createListPlugins,createListEventPlugin, etc.) in one place greatly simplifies the import process. However, consider documenting in the code or a README which plugins should typically be used together, to assist users who might be uncertain about how to combine them for common functionality.packages/editor/src/core/extensions/flat-list/core/commands/keymap.ts (2)
49-60: Potential for code duplication
deleteCommandis very similar tobackspaceCommand. If the logic between the two grows further, you might consider refactoring common steps into a shared utility, differentiating them only where behavior actually diverges.
62-83: Helpful keymap objectCentralizing keybindings in
listKeymapis a good design pattern. This consolidates the user experience in one place, making future modifications more straightforward. Consider adding support for platform-specific shortcuts (e.g., Mac vs. Windows) if your editor usage spans multiple systems, or at least comment about potential differences.packages/editor/src/core/extensions/flat-list/core/schema/to-dom.ts (1)
76-88:defaultMarkerGetteris straightforwardThe function effectively handles task and toggle list markers. If you extend more list types in the future, consider a fallback approach (e.g., a standard bullet). Currently, returning
nullor an empty array is a reasonable approach but might need handling if additional list kinds arise.packages/editor/src/core/extensions/read-only-extensions.tsx (1)
142-142: Appending the extension at the endAdding
FlatListExtensionto the read-only editor extensions ensures it’s available as part of the final returned array, preserving the original order of other extensions. If the initialization order for extensions could matter, consider providing a quick comment or docs on why it appears last.packages/editor/src/core/extensions/side-menu.tsx (2)
125-128: Ensure offset constants are well-documented and validated.Here, the logic modifies
rect.leftandrect.topconditionally for.prosemirror-flat-list. While this helps tweak the side menu positioning, consider defining constants or enumerations for these numeric offsets (e.g., 5, 6) to improve maintainability, reduce magic numbers, and easily adjust them as needed.
134-136: Consider merging duplicate offset logic across branches.Lines 134-136 also apply offsets if
node.classList.contains("prosemirror-flat-list"). This mirrors the logic in lines 125-128 (though with slightly different offset values). Consider consolidating these conditions into a shared helper or applying them in a single block (if feasible) to avoid inconsistencies and duplication.packages/editor/src/core/components/menus/menu-items.ts (4)
25-25: Consider consistent naming for imported icons.
ListCollapseis imported here among many icons. The name is meaningful, but ensure that the naming stays consistent with the rest of the list-related icons (ListIcon,ListOrderedIcon) so it's immediately recognizable during refactoring or UI changes.
179-185: New ToggleListItem introduced.This new menu item for toggle lists is coherent with the bullet and ordered lists approach. Make sure to double-check styling or UI states so that toggling appears consistent with other list types.
186-186: Ensure spacing or separation after new list items.Having an additional blank line (line 186) can be removed or clarified. If it’s intentional for code readability, that’s fine; if not, consider removing to keep consistent formatting in the file.
270-270: Menu ordering for ToggleListItem.Placing
ToggleListItemnearBulletListItemandNumberedListItemmight boost discoverability for users. Consider reordering if a logical grouping of list items helps the UI/UX.packages/editor/src/core/helpers/editor-commands.ts (5)
66-66: Check for consistency with surrounding functions.Line 66 is empty; remove if not needed or keep if it logically separates functions for clarity.
79-79: Maintain consistent spacing or remove redundant line.Similar to line 66, consider removing if not necessary for readability.
92-92: Consider removing redundant blank line.If this spacing is not purposeful, removing it would maintain consistency.
105-105: Consider grouping blank lines consistently.
Similar spacing feedback as above lines.
177-178: Removal of clearNodes() before table insertion is beneficial for preserving user content.The revised approach avoids unnecessarily clearing nodes. Ensure that this doesn’t cause unexpected interactions if the user tries inserting a table inside existing container nodes. Possibly add tests for partial table insertions to confirm correct behavior.
packages/editor/src/core/extensions/typography/index.ts (1)
83-85: Remove or document deprecated rule.
Commenting outraquosuggests it is deprecated or unused. Consider removing it altogether if no longer needed, or provide a comment explaining any future re-enablement.packages/editor/src/core/extensions/flat-list/core/style.css (3)
7-7: Remove duplicate margin-bottom declaration.The
margin-bottomproperty is declared twice (lines 5 and 7).margin-top: 0; margin-bottom: 0; margin-left: 32px; - margin-bottom: 0; position: relative;
17-60: Well-implemented ordered list counter management.The implementation handles ordered list counters effectively:
- Uses
contain: styleto prevent counter leakage- Supports custom order numbers with feature detection
- Handles Safari compatibility for older versions
Consider adding a CSS custom property for the counter name (e.g.,
--list-counter-name) to make it more maintainable and reusable across different list types.
102-104: Consider transition for collapsed state.Adding a transition effect when collapsing/expanding toggle lists would improve the user experience.
&[data-list-collapsable][data-list-collapsed] > .list-content > *:nth-child(n + 2) { display: none; + transition: height 0.2s ease-out; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (82)
packages/editor/package.json(2 hunks)packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx(2 hunks)packages/editor/src/core/components/menus/menu-items.ts(4 hunks)packages/editor/src/core/extensions/custom-list-keymap/list-keymap.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-huh.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-weird.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor-working.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor.optimized.ts(1 hunks)packages/editor/src/core/extensions/drop-cursor.ts(1 hunks)packages/editor/src/core/extensions/extensions.tsx(5 hunks)packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/enter-without-lift.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/indent-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/join-collapsed-backward.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/join-list-up.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/join-textblocks-around.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/keymap.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/move-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/protect-collapsed.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/set-safe-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/split-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/toggle-collapsed.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/toggle-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/unwrap-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/wrap-in-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/dom-events.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/input-rule.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/migrate.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/node-view.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/clipboard.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/event.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/rendering.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/plugins/safari-workaround.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/schema/node-spec.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/schema/parse-dom.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/schema/to-dom.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/style.css(1 hunks)packages/editor/src/core/extensions/flat-list/core/types.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-end.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-start.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/auto-fix-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/block-boundary.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/browser.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/create-and-fill.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/cut-by-index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/get-list-type.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/in-collapsed-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-block-node-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-collapsed-list-node.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-list-node.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-list-type.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-node-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/is-text-selection.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/list-range.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/map-pos.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/max-open.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/parse-integer.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/patch-command.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/range-to-string.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/safe-lift.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/set-list-attributes.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/set-node-attributes.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/unwrap-list-slice.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/utils/zoom-in-range.ts(1 hunks)packages/editor/src/core/extensions/flat-list/heading-list-extension.ts(1 hunks)packages/editor/src/core/extensions/flat-list/list-extension.ts(1 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(2 hunks)packages/editor/src/core/extensions/side-menu.tsx(1 hunks)packages/editor/src/core/extensions/table/table/table.ts(1 hunks)packages/editor/src/core/extensions/typography/index.ts(1 hunks)packages/editor/src/core/helpers/editor-commands.ts(2 hunks)packages/editor/src/core/hooks/use-editor.ts(2 hunks)packages/editor/src/core/plugins/drag-handle.ts(2 hunks)packages/editor/src/core/types/editor.ts(1 hunks)packages/editor/src/index.ts(1 hunks)packages/editor/src/styles/drag-drop.css(1 hunks)packages/editor/src/styles/editor.css(1 hunks)
⛔ Files not processed due to max files limit (2)
- packages/editor/src/styles/lists.css
- web/core/constants/editor.ts
✅ Files skipped from review due to trivial changes (1)
- packages/editor/src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/editor/src/core/extensions/flat-list/core/utils/browser.ts
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-weird.ts
[error] 212-212: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 226-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-working.ts
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 246-246: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor.ts
[error] 266-266: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 272-272: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-huh.ts
[error] 189-189: expected , but instead found :
Remove :
(parse)
[error] 189-189: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 196-196: expected , but instead found :
Remove :
(parse)
[error] 196-196: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 207-207: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 268-268: expected , but instead found :
Remove :
(parse)
[error] 268-268: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 273-273: expected , but instead found :
Remove :
(parse)
[error] 273-273: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 305-305: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 309-309: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 313-313: expected , but instead found :
Remove :
(parse)
[error] 313-313: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 320-320: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 320-320: expected , but instead found :
Remove :
(parse)
[error] 320-320: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 326-326: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 326-326: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 330-331: expected a semicolon to end the class property, but found none
(parse)
[error] 331-331: expected a semicolon to end the class property, but found none
(parse)
[error] 345-346: expected a semicolon to end the class property, but found none
(parse)
[error] 355-356: expected a semicolon to end the class property, but found none
(parse)
[error] 364-364: expected } but instead the file ends
the file ends here
(parse)
[error] 281-281: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 295-295: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 355-356: Duplicate class member name "function"
(lint/suspicious/noDuplicateClassMembers)
[error] 189-194: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 196-205: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 268-271: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 305-307: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 309-311: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 326-328: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts
[error] 18-18: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 20-20: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 27-27: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 29-29: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/editor/src/core/extensions/drop-cursor.optimized.ts
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 277-277: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts
[error] 31-34: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 32-32: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 33-33: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 61-61: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts
[error] 239-239: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 254-254: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (134)
packages/editor/src/core/extensions/table/table/table.ts (4)
114-114: Good insertion node creation.
ThecreateTableutility usage is straightforward and aligns well with the schema. No issues observed here.
116-117: Check insertion position correctness.
Calculatingpositionusingselection.$from.before(selection.$from.depth)is typically correct for many scenarios. However, ensure that if the selection is nested (e.g., within a list), this logic still places the table where intended.
126-127: Insertion of the table is appropriate.
Inserting the created table at the computed position is consistent with typical ProseMirror patterns. No immediate concerns.
129-135: Validate cursor positioning after table insertion.
Setting the selection to(cellPos + 1)may work for basic scenarios, but ensure thorough testing for correct cursor placement in different editor states (e.g., nested nodes, multiple tables).packages/editor/src/core/extensions/flat-list/core/schema/parse-dom.ts (2)
1-2: Imports look good.
Everything here is necessary for defining and exporting the parse rules for lists, with no apparent issues.
109-119: Hard-coding nested lists tobulletkind.
By defaulting nested lists to bullet, you risk losing the original context if the source was anolor a toggle list. Verify that this behavior is desired for converting Dropbox Paper invalid HTML. Otherwise, consider allowing a more graceful fallback that inspects the node and adjusts accordingly.✅ Verification successful
Let me gather more information about the list handling in the codebase.
Let me gather one final piece of information to understand how list kinds are handled in the codebase.
Defaulting nested lists to
bulletis intentional for Dropbox Paper compatibilityThe code explicitly handles invalid HTML structure from Dropbox Paper where nested lists can appear directly under
ulorolelements. The decision to default these to bullet lists appears to be a deliberate choice for compatibility, as:
- The comment in the code clearly states this is specifically for handling Dropbox Paper's invalid HTML structure
- The codebase has proper handling for other list types (ordered, task, toggle) in valid HTML scenarios
- There's no evidence of lost functionality, as regular nested lists are handled through proper DOM parsing rules elsewhere in the codebase
The current implementation provides a consistent way to handle this edge case while maintaining compatibility with content pasted from Dropbox Paper.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for places that transform nested lists to bullet # to confirm it's the only desired behavior across the codebase. rg -A 4 'nested lists'Length of output: 973
Script:
#!/bin/bash # Check for list-related code and attributes ast-grep --pattern 'kind: "bullet"' # Look for other list parsing or transformation logic rg -A 4 'ListAttributes' # Check for Dropbox Paper related code rg -B 2 -A 2 'dropbox|paper'Length of output: 32055
Script:
#!/bin/bash # Check how list kinds are determined from HTML elements ast-grep --pattern 'tag: ":is(ul, ol)"' # Look for list kind determination logic rg -A 4 'kind: "ordered"'Length of output: 3497
packages/editor/src/core/extensions/flat-list/core/utils/range-to-string.ts (3)
1-2: Imports look good here.
The imports correctly reference the necessary types from@tiptap/pm/model.
3-4: EnsurecutByIndexhas the required test coverage.
SincerangeToStringdepends oncutByIndex, please verify thatcutByIndexcorrectly handles out-of-bounds indices and other edge cases.
5-9: Documentation is clear and concise.
Your JSDoc comment is well structured, providing a clear explanation of the utility’s purpose and internal scope.packages/editor/src/core/extensions/flat-list/core/utils/auto-fix-list.ts (4)
9-26: Potential infinite loop in generator.
This generator runs in an infinitewhile (true)loop, which may lead to unintended repeated yields if called more times than expected. Confirm that consumers of this generator are designed to iterate a limited number of times and handle any leftover mapping operations.Would you like me to generate a script to scan for all call sites of
getTransactionRangesto ensure it’s used in a controlled manner?
29-63: Validate descending boundary sort logic.
The boundaries are sorted in descending order to ensure correct join application from the bottom up. Make sure this order is intentional and verified by tests.
65-67: List join condition looks appropriate.
UsingisListNodechecks ensures that only adjacent list nodes (plus the child) are joined.
103-104: Integration withpatchCommandis well-structured.
Good use of a wrapper to seamlessly integrate the auto-fix functionality into the command ecosystem.packages/editor/src/core/extensions/custom-list-keymap/list-keymap.ts (2)
46-46: Validate default Tab behavior outside List context.
Returningfalsehere allows Tab to bubble up, enabling standard focus navigation when the editor is not in alistItemortaskItem. Verify that this behavior is consistent with the desired user experience, particularly if the rest of your app relies on Tab for a different workflow.
54-54: Confirm Shift-Tab logic aligns with desired user experience.
Similarly, returningfalseforShift-Taboutside list contexts will permit the browser to handle backward focus traversal. Ensure this is intended and doesn't conflict with handling indentation/unindentation in other parts of the code.packages/editor/src/core/extensions/flat-list/core/commands/unwrap-list.ts (1)
29-62: Verify correctness for partial or empty list selections.
The main unwrapping logic checks for valid node or range selections but might behave unexpectedly if the selected list node has no content or is partially formed. Consider adding tests to ensure stability in edge cases.Would you like assistance generating a suite of edge case unit tests for this scenario?
packages/editor/src/core/extensions/flat-list/core/commands/move-list.ts (1)
28-86: Ensure thorough testing of boundary conditions.
The logic for moving list items up and down is complex and index-based. Confirm that boundary indexes (startIndex = 0orendIndex = parent.childCount) are handled gracefully.Automated tests covering corner cases can prevent off-by-one or out-of-bounds errors. Would you like a shell script to scan for existing tests related to list movement?
packages/editor/src/core/extensions/flat-list/core/migrate.ts (1)
72-82: Add robust tests for list migrations.
While the recursive approach covers bullet, ordered, and task lists, confirm it behaves correctly for nested or partial lists, and for edge cases like empty nodes.I can help create a script to search for existing migration tests or generate new test coverage for these scenarios if desired.
packages/editor/src/core/extensions/flat-list/core/commands/wrap-in-list.ts (1)
37-45: Verify deeper parent node logic.By shifting the
range.depthby one level if the parent is a list node, the command ensures wrapping at the correct level. However, consider verifying corner cases (e.g., wrapping nested lists or deeply nested nodes) to confirm no unexpected range adjustments occur.packages/editor/src/core/extensions/flat-list/core/input-rule.ts (2)
38-59: Confirm match index usability.When retrieving old attributes and computing
newAttrs, ensure the index usage onmatchremains valid in all cases (e.g., empty or partial regex matches). An out-of-bounds reference can lead to undefined behavior.
82-103: Avoid conflicting input rules.The predefined
listInputRulesmight compete with other custom or default Tiptap input rules (e.g., smart quotes, code block triggers). Double-check that your patterns do not conflict with existing input rules, especially if multiple rules share overlapping conditions.packages/editor/src/core/extensions/flat-list/list-extension.ts (3)
50-77: Commands naming consistency.The commands
createList,indentList,dedentList, andsplitListconsistently map to the underlyingcreateWrapInListCommand,createIndentListCommand, etc. This is clean and aligns nicely with your extension's aim. Good practice to keep them in a single place for clarity.
78-107: Potential keymapping collisions.The shortcut handlers (
Tab,Shift-Tab,Enter) override default behaviors if the list extension is active. Ensure that this doesn't inadvertently remove necessary functionality for other node types or cause collisions with higher-level keymaps. Possibly document or confirm other node types won't need these keys.
108-111: Composing plugins with caution.The array returned by
addProseMirrorPluginsmerges your custom plugin set withcreateListPlugins(...),listKeymapPlugin, andlistInputRulePlugin. This approach is excellent. Just watch out for potential plugin ordering pitfalls if other third-party plugins rely on matching or transforming list nodes.packages/editor/src/core/extensions/flat-list/heading-list-extension.ts (1)
99-148: Keybinding overrides.Your heading list extension uses a similar approach to the list extension to override
Tab,Shift-Tab,Enter, etc. If the heading list node is active, standard text indentation or new line behavior is replaced. Validate that these overrides do not block normal heading operations or accessibility-based shortcuts.packages/editor/src/core/extensions/flat-list/core/commands/indent-list.ts (1)
57-63: Return consistency after indentation.When
indentRangesucceeds on line 58, you dispatch and return early. This is good, but ensure the calling command also handles any partial indentation. In more complex scenarios (e.g., partial success near the edges), a direct single return could miss further indentation steps. Confirm that partial ranges are correctly updated before returning.packages/editor/src/core/extensions/extensions.tsx (6)
6-11: Verify consistent naming across list-related imports.All list extensions (ListItem, BulletList, OrderedList) have consistent naming and are properly imported. No issues detected here.
40-43: Ensure separation of concerns for DropCursorExtension and FlatListExtension.The newly imported
DropCursorExtensionandFlatListExtensionare related but serve different purposes. Confirm they don’t overlap in functionality or create conflicts when used together, especially in drag/drop scenarios involving new flat list features.
77-80: Assess disabling the dropcursor in StarterKit.Previously, dropcursor was customized via CSS classes. Now it’s disabled (
dropcursor: false) here. Verify whether the newDropCursorExtensionfully replaces that functionality to avoid conflicting or redundant dropcursor behaviors.
83-84: Validate the combined usage of DropCursorExtension and FlatListExtension.You’re integrating both extensions into the same editor. Double-check collisions in event handling, especially with list-specific drag/drop gestures in
FlatListExtension.
121-133: Review nested TaskItem usage.
TaskItemis configured withnested: true. Verify the editor’s behavior for deeply nested tasks (e.g., a TaskItem within a TaskItem) and whether it’s intended or can lead to undesired complexities.
207-212: Check Markdown integration for new list extensions.Markdown extension is enabled with
html: true. Verify that bullet/ordered/task lists parse correctly from and into Markdown, especially after adopting the new “flat list” approach.packages/editor/src/core/extensions/flat-list/core/commands/split-list.ts (4)
22-24: Confirm correct chaining of the split commands.
createSplitListCommandcomposessplitBlockNodeSelectionInListCommandandsplitListCommandwithwithAutoFixList. Ensure no edge cases where both commands might conflict or produce unintended splits.
31-65: Validate behavior of splitBlockNodeSelectionInListCommand.The command only applies if:
- The selection is a block node selection
- The parent is a list
- The parent has exactly one child
- That single child is not a list node
Double-check corner cases, e.g., when a list has only a single nested child but that child also contains nested content.
67-130: Ensure logical splitting rules for nested lists.
splitListCommanddedents or splits based on cursor position (first child vs. subsequent child). Validate that these heuristics operate correctly for advanced nesting scenarios.
132-185: Accurately handle new list creation within doSplitList.
doSplitListdetermines whether to create an additional block type or a new list node. Review correctness for boundary conditions (split at start/end of a list) and howattrs.collapsedinfluences it.packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts (5)
1-4: Confirm safety of ReplaceAroundStep usage.
ReplaceAroundStepcan be tricky for transformations. Ensure that surrounding content and indexes remain valid after dedenting multiple nested lists in large documents.
43-63: Ensure single-step or multi-step dedent logic.
dedentListCommandfinds the lists range and callsdedentRange. Test multi-nested lists thoroughly to confirm that repeated calls or partial dedenting doesn’t produce malformed structures.
109-148: Confirm dedentNodeRange logic.
dedentNodeRangecallssafeLiftRangeordedentOutOfList. Evaluate ifsafeLiftRangemight incorrectly lift a node that was partially dedented, especially with nested list items.
211-233: Double-check boundary fix logic.
fixEndBoundarymerges siblings by callingmoveRangeSiblings. Confirm that multi-layer merges still respect user expectations of separate list blocks.
235-279: Validate dedentOutOfList merges.Looping over child nodes merges them into one “big list node,” then uses
ReplaceAroundStep. This might produce surprising results if partial merges were intended. Thoroughly test.packages/editor/src/core/hooks/use-editor.ts (2)
27-27: Check for potential conflicts in your migration logic.
migrateDocJSONis newly imported. Ensure the data structure post-migration stays compatible with other features (like indentation commands).
114-115: Prevent repeated migrations.
[hasMigrated, setHasMigrated]ensures we only migrate once. This is good, but watch out for scenarios where re-migration might be needed if the doc changes drastically (e.g., loaded from a stale source).packages/editor/src/core/extensions/drop-cursor-somewhat-working.ts (2)
125-142: Ensure dynamic event listeners are cleaned up.
DropCursorViewattaches drag events in the constructor, removing them indestroy(). This is correct, but confirm no memory leaks if the extension re-initializes multiple times.
268-302: Validate custom drop logic for list items.When a user drops a node between lists, the code re-inserts that node via
tr.delete+tr.insert. Thoroughly test to confirm that merges or splits of lists happen as intended, especially if dropping multiple items.packages/editor/src/core/extensions/drop-cursor-working.ts (1)
214-225: Verify the sibling detection logic.When determining the next sibling or parent node in the DOM, ensure that edge cases (e.g.,
nullor missing elements) are gracefully handled to prevent runtime errors or incorrect discount logic for the drop position.🧰 Tools
🪛 Biome (1.9.4)
[error] 222-222: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/editor/src/core/extensions/drop-cursor-huh.ts (1)
76-77: Ensure offset usage is correct.Subtracting
1frompositionor applying offsets manually can lead to off-by-one errors. Double-check that these numeric adjustments reflect the intended final cursor position.packages/editor/src/core/extensions/drop-cursor.ts (1)
50-52: Confirm return type from the helper function.Your destructuring implies
rawIsBetweenFlatListsFnmay return an object with specific properties, but you also allow|| {}. Validate that the fallback prevents undefined references during object destructuring.packages/editor/src/core/extensions/drop-cursor.optimized.ts (1)
76-76: Double-check the final drop position offset.The code subtracts
2from the computeddropPosByDropCursorPosbefore insertion. Ensure this aligns with your content structure and does not cause off-by-one insertion.packages/editor/src/core/extensions/flat-list/core/utils/is-text-selection.ts (2)
1-2: No issues with import statement.
3-5: Implementation looks good.
This type guard correctly checks if the value is aTextSelection. It is a precise and concise solution.packages/editor/src/core/extensions/flat-list/core/utils/is-list-type.ts (3)
1-2: No issues with initial import.
3-4: No issues with secondary import.
5-8: Verify multiple list type scenarios.
If the schema contains multiple list node types,getListType(type.schema) === typemight fail for some. Confirm there is only one list node type or handle multiple.packages/editor/src/core/extensions/flat-list/core/utils/parse-integer.ts (1)
1-7: Logic is sound.
The function correctly handles null and undefined cases, and cleanly returns an integer or null.packages/editor/src/core/extensions/flat-list/core/utils/is-list-node.ts (1)
6-9: This utility function looks clean and concise.The implementation is straightforward and checks for the truthiness of
nodebefore callingisListType. This is both safe and purposeful. No changes needed at this time.packages/editor/src/core/extensions/flat-list/core/utils/is-block-node-selection.ts (1)
5-7: Implementation is appropriate for selection checks.Using
isNodeSelection(selection)before verifyingselection.node.type.isBlockis a clear approach. This guarded check ensures no runtime error for undefined node types.packages/editor/src/core/extensions/flat-list/core/utils/is-collapsed-list-node.ts (1)
8-9: Logic is aligned with intended functionality.The function checks both the node type and the
collapsedattribute, making it easy to detect collapsed list nodes. No issues found.packages/editor/src/core/extensions/flat-list/core/plugins/rendering.ts (1)
10-18: Logical and modular plugin creation.Wrapping the node view logic in a separate plugin (
createListRenderingPlugin) is a clean approach, ensuring the rendering concerns stay modular. This fosters future extensibility for different list node types or advanced behaviors.packages/editor/src/core/extensions/flat-list/core/utils/patch-command.ts (1)
3-11: Clarify assumptions for thepatchfunction.This utility assumes that the
patchfunction always returns a transaction fully compatible with thedispatchcallback. Consider validating or documenting preconditions (e.g., ensuring the returned transaction is properly shaped) to avoid subtle runtime errors ifpatchis malformed.packages/editor/src/core/extensions/flat-list/core/plugins/event.ts (1)
10-18: Verify mouse event behavior for different node types.The plugin handles
mousedownevents on list markers but might also intercept interactions with other child nodes. Verify thathandleListMarkerMouseDowngracefully ignores or properly handles clicks outside the expected context (e.g., nested nodes or embedded media).packages/editor/src/core/extensions/flat-list/core/utils/set-node-attributes.ts (1)
4-13: Well-structured utility function for updating node attributes.This function cleanly updates node attributes only when necessary, returning a
booleanindicating whether any changes were made. This approach helps avoid unnecessary transactions and keeps performance overhead low.packages/editor/src/core/extensions/flat-list/core/plugins/safari-workaround.ts (1)
12-14: Safari IME plugin looks solid.The plugin elegantly wraps
imeSpanand can be cleanly integrated into your editor plugin stack. Ensure you document usage in your setup to inform developers of its role in managing Safari’s IME quirks.packages/editor/src/core/extensions/flat-list/core/utils/create-and-fill.ts (1)
3-15: Effective use of ProseMirror’screateAndFill.This function robustly handles node creation and ensures validity via
node.check(). The explicitRangeErrormessage for failed creation is helpful for debugging.packages/editor/src/core/extensions/flat-list/core/utils/in-collapsed-list.ts (1)
7-18: Implementation looks good.This function correctly checks each ancestor node for the
collapsedattribute. The loop structure is efficient for typical ProseMirror documents, as the maximum depth is generally small. No critical issues or off-by-one errors are apparent.packages/editor/src/core/extensions/flat-list/core/utils/zoom-in-range.ts (1)
6-19: Consider verifying deeper edge cases in complex nested structures.This utility returns a deeper block range if one exists. Be mindful of extreme nesting scenarios (e.g., deeply nested lists or tables). Though rare, they can occur in rich text editors, so add tests to cover extreme nested levels to ensure correctness.
packages/editor/src/core/extensions/flat-list/core/utils/at-textblock-start.ts (1)
6-10: Implementation is correct and concise.The method properly checks whether the cursor is at the start of the text block. No improvements required; the logic and fallback to
nullfor non-text cursor selections are appropriate.packages/editor/src/core/extensions/flat-list/core/types.ts (3)
8-8: LeverageListKindfor consistency.
Good job using a union type for meaningful enumeration of valid list kinds:"bullet" | "ordered" | "task" | "toggle". This helps ensure type safety across your list-related utilities.
19-25: Validate theProsemirrorNodeJSONinterface.
This interface seems well-aligned with ProseMirror node definitions. Ensure that any custom attributes outside ofAttrsare carefully handled in your node transformations to avoid unexpected runtime errors.
27-27: Re-exportingNodeasProsemirrorNodeis helpful for clarity.
This re-export clarifies that we're dealing with ProseMirror’sNode, which can reduce confusion in your codebase. Nicely done.packages/editor/src/core/extensions/flat-list/core/plugins/clipboard.ts (2)
1-2: Confirm that the schema is fully aligned with list node specs.
The plugin relies on schema specs for list nodes. Ensure all relevant schemas (e.g., bullet, ordered, task, toggle) are well-defined so the clipboard plugin serializes/deserializes them accurately.
13-20: Excellent approach to customizing clipboard handling for lists.
Using a specialized serializer (ListDOMSerializer) andunwrapListSlicefosters consistent copy-paste behavior for various list types. This design encapsulates list-clipboard logic neatly in one plugin.packages/editor/src/core/extensions/flat-list/core/utils/safe-lift.ts (1)
5-12: Safeguard edge cases when lifting.
safeLiftcorrectly checks for a validliftTargetbefore performing the lift, returningfalseif none is found. This approach prevents unsafe transformations and helps avoid runtime errors.packages/editor/src/core/extensions/flat-list/core/utils/set-list-attributes.ts (1)
8-22: Consider partial merges for extended attributes.
The current spread merging logic is straightforward, combining all existing attributes with the new ones. If you introduce nested attribute structures in the future, double-check deep merges so no data is inadvertently overwritten.Validate node type before merging attributes.
TheisListNodecheck is essential, but if you expand list node types beyond bullet/ordered/task/toggle, ensureisListNodehandles them uniformly.packages/editor/src/core/extensions/flat-list/core/utils/unwrap-list-slice.ts (1)
12-22: Implementation appears solid.The function effectively reduces open depth for slices containing a single list node, preventing unwanted deep nesting when pasting content. The loop operates on clear conditions and exits when they are no longer satisfied. This looks correct and efficient for most list-related use cases.
packages/editor/src/core/extensions/flat-list/core/utils/block-boundary.ts (2)
3-15: Validate the usage constraints.The loop uses
d <= $pos.depth, which is correct for typical scenarios. However, if an invalid depth is passed in (e.g., ifdepthis larger than$pos.depth), it could result in unexpected logic flow. Recommend documenting valid depth ranges to ensure consistency.
17-29: Logic is clear and aligns with block boundary checks.Similar to the start boundary check, this approach for checking the end boundary is well-structured, iterating up the node hierarchy without redundancy. The readability is good, and the function’s name matches its behavior.
packages/editor/src/core/extensions/flat-list/core/commands/toggle-list.ts (1)
16-27: Toggling logic looks coherent.Chaining
unwrapListfollowed bywrapInListis a common pattern for toggling. However, verify that lists of the same kind are correctly re-wrapped or unwrapped without unexpected side effects. Test thoroughly on multiple list kinds (e.g., bullet, ordered) to confirm correct toggling behavior.packages/editor/src/core/extensions/flat-list/core/utils/max-open.ts (3)
1-2: Imports look good.
No issues with these import statements; they correctly pull in the required ProseMirror types.
3-17: Reference to original ProseMirror code is commendable.
Copying relevant snippets from ProseMirror ensures consistent behavior. The logic for incrementally increasingopenStartlooks correct, and the optionalopenIsolatingflag is well-handled.
19-33: Symmetrical approach for maxOpenEnd is consistent with maxOpenStart.
Mirroring the logic inmaxOpenStartensures that both ends of the fragment are handled similarly. The code is clear, succinct, and follows best practices.packages/editor/src/core/extensions/flat-list/core/utils/list-range.ts (3)
1-4: Helper imports are well-organized.
TheNodeRange,ResolvedPos, andisListNodeimports are concise and relevant, aligning with the utility's purpose.
5-31: Robust approach for finding list ranges.
ThefindListsRangefunction carefully re-checks ranges with adjusted depths, ensuring that multiple sibling list nodes are captured. The recursive fallback of$to<$fromis succinct and effective.
33-44: Efficient validation of list nodes.
TheisListsRangefunction loops through children in the specified range to confirm they are lists. This explicit check prevents mistakenly treating non-list nodes as lists.packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts (1)
1-9: Overall structure is clear.
The function commentary is thorough and explains the rationale behind splitting logic and boundary avoidance.packages/editor/src/core/extensions/flat-list/core/plugins/index.ts (3)
1-3: Appropriate imports for plugin creation.
These imports align with the modular plugin architecture.
4-8: Plugins are well-organized by feature.
Clipboard, event, rendering, and Safari workarounds are separated, enhancing clarity.
31-36: Additional exports for plugin creation.
Exporting individual plugin builders promotes flexibility and reusability for advanced consumers.packages/editor/src/core/extensions/flat-list/core/commands/protect-collapsed.ts (1)
19-43: Check for multi-range selections.
This logic currently considers single continuous ranges. If multi-range selections or disjoint ranges become possible, you might need additional handling.Generate a script to search for multi-range usage:
packages/editor/src/core/extensions/flat-list/core/node-view.ts (1)
23-25: Ensure toggling logic is robust across browsers.
Adding an empty<span>is a clever workaround for iOS Safari, but watch for potential quirks on other platforms. Consider adding browser checks, if new anomalies arise.packages/editor/src/core/extensions/flat-list/core/schema/node-spec.ts (1)
39-52: Validate attributes for complex use cases.
collapsed,checked, etc. can be interdependent attributes. Ensure there's a plan for thoroughly testing combinations (e.g., toggled and collapsed).packages/editor/src/core/extensions/flat-list/core/commands/toggle-collapsed.ts (3)
1-5: Imports look good.
All imported modules appear essential; no issues identified.
8-23: Appropriate interface design.
TheToggleCollapsedOptionsinterface is well-structured, with clear documentation for optional fields.
56-64: Well-defined default toggleability check.
ThedefaultIsToggleablefunction correctly ensures toggling only applies to toggle lists that contain children.packages/editor/src/core/extensions/flat-list/core/commands/join-list-up.ts (5)
1-7: Imports are standard and concise.
No redundant imports detected.
8-18: Adequate documentation.
The docstring thoroughly conveys the command's behavior under different conditions.
19-47: Check boundary cases injoinListUp.
While the logic for index checks (indexInList === 0orindexInList === listNode.childCount - 1) is correct, consider a unit test for edge cases, such as a list with a single child, or scenarios where the view might not supply a valid cursor.
49-64:liftListContentutility is well-structured.
Clear separation of concerns and usage ofNodeRangewithsafeLift.
66-76:liftParentfunction matches intended use.
Logic is straightforward and reusessafeLift; no concerns here.packages/editor/src/core/extensions/flat-list/core/dom-events.ts (2)
1-9: Imports are well-structured.
No concerns regarding import statements.
52-65: Handler logic is straightforward.
defaultListClickHandlertogglescheckedorcollapsedbased onkind. This is a concise approach, and the expansions for other list kinds remain flexible.packages/editor/src/core/extensions/flat-list/core/commands/set-safe-selection.ts (5)
1-11: Imports are appropriate.
The modules and functions imported here align well with the file’s purpose.
12-24: Selection handling logic is well-thought-out.
moveOutOfCollapsediterates through depths to ensure the user never gets stuck in a collapsed node. This is beneficial for user experience.
26-41:setSafeSelectionapproach is coherent.
Combining the checks for$fromand$toensures the final selection is outside of collapsed nodes.
42-42: Convenient command wrapper export.
withSafeSelectionnicely follows the existing pattern of patching commands for safer operations.
44-51:getCollapsedPositionnaming is clear.
This function effectively returns the position before the collapsed node. No issues found.packages/editor/src/core/extensions/flat-list/core/index.ts (1)
1-9: Export structure looks goodThe grouping of commands, DOM event handlers, and utilities into separate modules helps maintain a logical separation of responsibilities. This approach will make your code more discoverable and easier to maintain.
packages/editor/src/core/extensions/flat-list/core/commands/keymap.ts (3)
1-8: Organized import structureThese ProseMirror command imports are well-organized. Keeping them together at the top provides clarity on which default commands are leveraged and extended.
17-26: Inline documentation is clearThe docstring explaining the
enterCommandis well-structured and accurately references the commands it composes. Good job making it easy for other developers to understand how these commands work together.
27-47: Complex command chaining logicThe
backspaceCommandwraps several commands for robust handling of list scenarios. While this is powerful, observe that further additions or special cases can make the chain unwieldy. Consider writing comprehensive tests covering partial deletions, empty lines, nested lists, etc., to ensure all branches behave correctly.Would you like me to generate some example test scaffolding or shell scripts to confirm coverage of these edge cases?
packages/editor/src/core/extensions/flat-list/core/schema/to-dom.ts (3)
1-31: Clear interface definition
ListToDOMOptionsis well-defined with descriptive docstrings. This ensures clarity for anyone working with your DOM conversion logic.
33-74: Conditional logic for native vs. custom list renderingThe
listToDOMfunction elegantly handles both native lists and custom<div>containers. However, be mindful that advanced manipulations (like stylized markers or toggles) in a native list might cause unexpected browser behavior in some edge cases. Testing in multiple browsers (Safari, IE, etc.) is recommended to ensure consistent rendering.
90-106: Data attributes for styling and togglingThe
defaultAttributesGetterensures that key data attributes (data-list-kind,data-list-collapsed, etc.) are attached for styling or DOM-based logic. Be mindful of potential naming collisions if other components or frameworks read or modify the same attributes.packages/editor/src/core/extensions/read-only-extensions.tsx (1)
34-34: ImportingFlatListExtensionThanks for keeping the import at a consistent location alongside other extension imports. This helps maintain a clear import hierarchy.
packages/editor/src/core/types/editor.ts (1)
38-38: Addition of "toggle-list" command looks good.This addition appropriately extends the editor commands to support toggling list functionality. Ensure all references to
"toggle-list"are thoroughly tested and invoked appropriately throughout the codebase.packages/editor/src/core/components/menus/menu-items.ts (4)
38-41: List toggling methods aligned with "flat" approach.The newly imported
toggleFlatBulletList,toggleFlatOrderedList,toggleFlatTaskList, andtoggleFlatToggleListunify the list creation approach. This consistency is a good improvement. Verify that any references to the older toggles (toggleBulletList, etc.) are removed to avoid unused methods.
158-159: Use of isActive("list", { kind: "bullet" }) in BulletListItem.Switching from
isActive("bulletList")toisActive("list", { kind: "bullet" })is a clear, extensible approach that aligns with the new list architecture. This looks good.
166-167: Use of isActive("list", { kind: "ordered" }) in NumberedListItem.Similar to bullet lists, adopting the new
listscope for ordered lists is consistent and follows the architectural change for "flat" lists.
174-175: Refactoring to use isActive("list", { kind: "task" }) in TodoListItem.Good alignment with the new "flat" list approach.
packages/editor/src/core/helpers/editor-commands.ts (4)
54-65: New toggleFlatOrderedList logic.This function introduces a clear approach for ordered lists via
createList. Confirm any existing calls totoggleOrderedListare now migrated, if the goal is to fully adopt this new approach.
67-78: New toggleFlatBulletList logic.Implementation is parallel to toggleFlatOrderedList. This is consistent with the new "flat" approach. Ensure range-based deletion performs correctly with multiline selections.
80-91: New toggleFlatTaskList logic.Again, consistent with the bullet/ordered approach. Great for uniform behavior across different list types.
93-104: New toggleFlatToggleList logic.Implementation mirrors the other "flat" expansions. Ensure extended toggles (like sub-items or nested toggles) handle range deletions properly if those are part of any roadmap capabilities.
packages/editor/src/core/plugins/drag-handle.ts (2)
23-23: New selector for flat-list is well-defined.
Including".prosemirror-flat-list"in the selectors is a clear addition for handling the new flat-list style blocks. Ensure this does not collide with any other existing CSS selectors that might unintentionally match.
382-383: Consider edge cases in position adjustment for flat-list.
Subtracting one from thedraggedNodePoswhen the node’s class name includes"prosemirror-flat-list"might cause off-by-one errors if combined with other list transformations (e.g., nested lists, toggling between list types). Please verify that the updated position remains accurate across all editor states.packages/editor/src/styles/drag-drop.css (1)
63-69: Confirm the styling offset for flat-list.
Applying a 25px horizontal offset and making the::afterborder transparent differentiates.prosemirror-flat-listfrom other selected nodes. This seems consistent with the new list type. Please verify that the increased offset does not introduce UI overlap in more complex layouts (e.g., nested lists, small viewport widths).packages/editor/package.json (4)
45-45: New bullet-list extension dependency.
Adding"@tiptap/extension-bullet-list"at version^2.10.3aligns with the new list functionality. Confirm that it does not conflict with any other versions in the project.
49-49: Update of list-item extension.
Upgrading"@tiptap/extension-list-item"from^2.1.13to^2.10.3is a significant jump. Verify any backward-incompatible changes in the extension and ensure they're accounted for in the new editor logic.
51-51: New ordered-list extension added.
Introducing"@tiptap/extension-ordered-list"is consistent with the new list enhancements. Validate that it integrates correctly with the bullet-list and list-item changes for a stable user experience.
69-69: Implement Safari IME fix.
The addition of"prosemirror-safari-ime-span"addresses known quirks in Safari’s IME handling. Check for any newly identified platform issues that may require additional patches.packages/editor/src/core/extensions/flat-list/core/style.css (2)
1-11: LGTM! Base list styles are well-structured.The base styles provide a good foundation with proper spacing and positioning. The use of logical properties (
margin-left) ensures better RTL support.
68-69: Consider browser support for logical height unit.The
1lhlogical height unit has limited browser support. Keep the fallback1.5emfor better compatibility.✅ Verification successful
The fallback for
1lhis already in placeLooking at the search results, I can see that there's already a fallback declaration
width: 1.5em;before thewidth: 1lh;in the CSS file. This follows the CSS cascade where browsers that don't support thelhunit will use theemunit fallback, while modern browsers that supportlhwill use that value. This is the recommended progressive enhancement approach.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check caniuse data for logical height unit support curl -s "https://raw.githubusercontent.com/Fyrd/caniuse/main/features-json/logical-height-units.json" | jq '.stats'Length of output: 177
Script:
#!/bin/bash # Search for any fallback declarations for lh unit in CSS files rg -B1 -A1 "width:\s*\d+lh" --type css # Search for any other usage of lh unit in CSS files rg "\d+lh" --type cssLength of output: 674
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (1)
21-21: LGTM! Clean integration of ToggleListItem.The
ToggleListItemis well-integrated into the existing menu structure, maintaining consistency with other list items.Also applies to: 46-46
packages/editor/src/styles/editor.css (1)
411-414: LGTM! Consistent paragraph spacing.The simplified padding approach provides more consistent spacing between paragraphs.
| // Delete any existing content at the current position if it's an empty paragraph | ||
| const nodeAfter = tr.doc.nodeAt(position); | ||
| if (nodeAfter && nodeAfter.type.name === "paragraph" && nodeAfter.content.size === 0) { | ||
| tr.delete(position, position + 2); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the hardcoded magic number "2" when deleting the empty paragraph.
Using a hardcoded length of "2" might break if the paragraph node structure changes. Consider using nodeAfter.nodeSize or a similar approach to ensure robust paragraph deletion.
- tr.delete(position, position + 2);
+ tr.delete(position, position + nodeAfter.nodeSize);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Delete any existing content at the current position if it's an empty paragraph | |
| const nodeAfter = tr.doc.nodeAt(position); | |
| if (nodeAfter && nodeAfter.type.name === "paragraph" && nodeAfter.content.size === 0) { | |
| tr.delete(position, position + 2); | |
| } | |
| // Delete any existing content at the current position if it's an empty paragraph | |
| const nodeAfter = tr.doc.nodeAt(position); | |
| if (nodeAfter && nodeAfter.type.name === "paragraph" && nodeAfter.content.size === 0) { | |
| tr.delete(position, position + nodeAfter.nodeSize); | |
| } |
packages/editor/src/core/extensions/flat-list/core/commands/join-collapsed-backward.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/utils/list-serializer.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/commands/dedent-list.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/utils/get-list-type.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/utils/split-boundary.ts
Show resolved
Hide resolved
packages/editor/src/core/extensions/flat-list/core/commands/join-textblocks-around.ts
Show resolved
Hide resolved
Palanikannan1437
force-pushed
the
feat/flat-lists
branch
from
fec5cc2 to
7536a78
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/editor/src/core/extensions/drop-cursor.ts (4)
264-268: Use optional chaining for safer access
Static analysis suggests using optional chaining to eliminate potential undefined access in lines involvingnodeandnode.type. Specifically, you might rewrite:-const disableDropCursor = node && node.type.spec.disableDropCursor; +const disableDropCursor = node?.type?.spec?.disableDropCursor;🧰 Tools
🪛 Biome (1.9.4)
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
270-274: Add null check or optional chaining for drag slice
Static analysis warns of potential undefined access when referencingthis.editorView.dragging.slice. Consider using optional chaining to safely guard this access:-const point = dropPoint(this.editorView.state.doc, target, this.editorView.dragging.slice); +const point = dropPoint(this.editorView.state.doc, target, this.editorView.dragging?.slice);🧰 Tools
🪛 Biome (1.9.4)
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
142-152: Refine arguments forsetCursor
The function signaturesetCursor(pos: number | null, isBetweenFlatLists?: boolean)carries a loosely typed second parameter. A more descriptive name or explicit interface for the cursor state might help maintain clarity, especially when adding new states or flags in the future.
154-194: Improve maintainability of overlay logic
TheupdateOverlaymethod is fairly large and has multiple branching conditions. Consider splitting out distinct responsibilities (e.g., computing rect positions, toggling classes, updating transform properties) into smaller helper functions for better readability.packages/editor/src/core/extensions/extensions.tsx (5)
60-62: Explicitly disabling built-in list configurationsBy setting
bulletList,orderedList, andlistItemtofalse, you are effectively deferring to custom or extended versions of these. This is a clean approach, although keep an eye on backward compatibility if existing features depended on Tiptap’s default lists.
85-93: CustomBulletListextension with no input rulesRemoving or overriding default input rules is common when implementing a more controlled list experience. If your project requires auto-list creation upon typing
-, consider adding minimal input rules or confirm your UI addresses this need.
94-102: CustomOrderedListextension with no input rulesSimilar to
BulletList, reintroduce or adapt input rules if users expect auto-incrementing numbered lists upon typing1.or similar patterns.
112-120: TaskList extension overrideFully custom
TaskListlogic might require more robust input or transformation rules if you want automatic creation of new tasks. Ensure user experience is intuitive without them.
121-133: CustomizingTaskItemwith keyboard shortcutsProviding an empty
addKeyboardShortcutsmight be an interim approach. If your usage scenario requires toggling tasks via keystrokes or pressing enter to automatically create new tasks, consider implementing these shortcuts for a smoother experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/editor/src/core/extensions/drop-cursor.ts(1 hunks)packages/editor/src/core/extensions/extensions.tsx(5 hunks)packages/editor/src/core/hooks/use-editor.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/hooks/use-editor.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/editor/src/core/extensions/drop-cursor.ts
[error] 265-265: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 271-271: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
packages/editor/src/core/extensions/drop-cursor.ts (3)
39-40: Validate data extraction prior torawIsBetweenFlatListsFncall
321-412: Confirm edge cases inrawIsBetweenFlatListsFn
Ensure the function properly handles scenarios where the element under drag is not part of a.prosemirror-flat-list, or if list logic is extended in the future (e.g., multiple nested toggles or tasks). You may want more robust checks to manage other edge cases, such as empty lists or partially rendered nodes.
430-452: Throttle logic looks good
The throttling for re-checking drag position appears reasonable. Confirm if the default threshold of 8px suits typical user interactions, otherwise consider making it configurable.packages/editor/src/core/extensions/extensions.tsx (8)
6-6: Good addition ofListItemimportImporting the
ListItemextension sets the foundation for advanced list handling. No immediate issues.
10-11: Proper bullet / ordered list importsUsing these explicit imports will help ensure clarity in the codebase and maintain separation of concerns for each list type. Nicely done.
40-41: Cleanly separating typesExplicitly importing
TExtensions,TFileHandler, andTMentionHandlerhelps maintain code clarity and type safety across the editor’s setup.
42-43: DropCursorExtension & FlatListExtension importsThese imports demonstrate a custom approach to drop-cursor behavior and advanced list features. Ensure there are no conflicts with the built-in Tiptap functionality or any duplicated logic.
77-80: Swapping the defaultdropcursorwithdropcursor: falseCommenting out the existing dropcursor class-based setup and defaulting to
dropcursor: falseis fine if the customDropCursorExtensionis fully tested. Monitoring user feedback on performance and drag-and-drop behavior is advisable.
83-84: Ensuring correct order of extension loadingPlacing
DropCursorExtensionandFlatListExtensionbefore the custom bullet/ordered/task list ensures the new drop-cursor behavior is applied early. Verify no unintended overrides occur between them.
103-111: Overriding defaultListItemextensionYou’re disabling default input rules. If certain behaviors (like pressing Enter to continue or break a list) are desired, ensure they remain accessible in your custom list or the
FlatListExtension.
207-212: Markdown extension with HTML transformationsEnabling
html: trueandtransformCopiedText / transformPastedTextcan make content editing more flexible, but also raises potential security and formatting concerns. Verify that any pasted content is sanitized or that your environment is trusted.
Palanikannan1437
force-pushed
the
feat/flat-lists
branch
from
96e9f15 to
8487bb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/editor/src/core/extensions/extensions.tsx (1)
85-120: Consider consolidating duplicate code in list extensions.The BulletList, OrderedList, and ListItem extensions have identical empty parseHTML and addInputRules methods. Consider creating a base extension or utility function to reduce code duplication.
+ const createEmptyListExtension = (extension: any) => + extension.extend({ + parseHTML() { + return []; + }, + addInputRules() { + return []; + }, + }); - BulletList.extend({ - parseHTML() { - return []; - }, - addInputRules() { - return []; - }, - }) + createEmptyListExtension(BulletList)packages/editor/src/core/extensions/index.ts (1)
26-26: Consider selective exports for better tree-shaking.
Exporting everything from"./flat-list"might introduce unwanted payload in the final bundle if only some parts of"./flat-list"are used.- export * from "./flat-list"; + export { + FlatListExtension, + // export additional symbols only as needed + } from "./flat-list";packages/editor/src/core/extensions/flat-list/list-extension.ts (2)
18-27: Ensure naming conventions remain consistent.
The interface is declared asflatListComponent, while the extension name is"list". Confirm that the naming aligns with the rest of the codebase for clarity and maintainability.
78-111: Add key bindings for backspace dedent/smart handling if needed.
Currently, Tab, Shift-Tab, and Enter are handled. Consider implementing a specialized backspace handler to remove indentation or merge items in certain cases, if that aligns with your design goals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/editor/src/core/extensions/extensions.tsx(5 hunks)packages/editor/src/core/extensions/flat-list/index.ts(1 hunks)packages/editor/src/core/extensions/flat-list/list-extension.ts(1 hunks)packages/editor/src/core/extensions/index.ts(1 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(4 hunks)packages/editor/src/core/extensions/table/table/table.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/editor/src/core/extensions/flat-list/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/src/core/extensions/read-only-extensions.tsx
- packages/editor/src/core/extensions/table/table/table.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-admin
- GitHub Check: build-space
🔇 Additional comments (7)
packages/editor/src/core/extensions/extensions.tsx (4)
2-5: LGTM! Imports are well-organized.The new imports are properly structured and align with the PR objectives for implementing flat lists.
Also applies to: 35-35
83-84: LGTM! New extensions are properly registered.The DropCursorExtension and FlatListExtension are correctly added to replace the disabled default extensions.
222-227: LGTM! Markdown configuration is well-configured.The Markdown extension is properly configured with HTML support and text transformation enabled.
60-62: Verify the impact of disabling dropcursor.While disabling the built-in list extensions is correct for implementing flat lists, disabling the dropcursor extension might affect the visual feedback when dragging content. Please ensure this doesn't impact the drag-and-drop user experience.
Also applies to: 77-80
✅ Verification successful
Disabling dropcursor is correct and intentional
The codebase implements its own comprehensive drag-and-drop system using @atlaskit/pragmatic-drag-and-drop with custom visual indicators, making the default ProseMirror dropcursor extension unnecessary and potentially conflicting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any custom drag-and-drop handling that might replace dropcursor functionality rg -l "onDragStart|onDragEnd|onDrop" --type ts --type tsxLength of output: 87
Script:
#!/bin/bash # Search for drag-and-drop handlers in both .ts and .tsx files echo "=== Searching for drag-and-drop handlers ===" rg -l "onDragStart|onDragEnd|onDrop" --type ts rg -l "onDragStart|onDragEnd|onDrop" -g "*.tsx" echo -e "\n=== Searching for dropcursor references ===" # Search for dropcursor-related code and configuration rg -i "dropcursor" --type ts rg -i "dropcursor" -g "*.tsx" echo -e "\n=== Checking for ProseMirror drag-drop plugins ===" # Look for any custom drag-drop implementations rg -i "drag|drop.*plugin" --type ts rg -i "drag|drop.*plugin" -g "*.tsx"Length of output: 67234
packages/editor/src/core/extensions/flat-list/list-extension.ts (3)
33-49: Check for potential read-only usage.
ThisNodeextension setsselectable: trueanddraggable: true. If the editor includes a read-only mode, confirm that the list extension respects read-only constraints appropriately.
50-77: Commands look well-structured.
This modular approach, splitting core list functionality into separate commands (create, indent, dedent, split), enhances maintainability. Good job!
1-16: Validate that all imported modules are in use.
Review the imports from"./core"to ensure no unused imports are dragged in. This helps maintain clarity and possibly reduce bundle size.✅ Verification successful
All imports from "./core" are actively used in the code
Each imported symbol is utilized within the file for type definitions, plugin creation, or command handling. No unused imports detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references of each imported symbol to verify usage rg -A 2 "ListAttributes|IndentListOptions|DedentListOptions|createListSpec|listKeymap|listInputRules|createWrapInListCommand|createIndentListCommand|createDedentListCommand|createSplitListCommand|createListPlugins"Length of output: 37777
| TaskItem.extend({ | ||
| parseHTML() { | ||
| return []; | ||
| }, | ||
| addInputRules() { | ||
| return []; | ||
| }, | ||
| addKeyboardShortcuts() { | ||
| return {}; | ||
| }, | ||
| }).configure({ | ||
| HTMLAttributes: { | ||
| class: "relative", | ||
| }, | ||
| nested: true, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement keyboard shortcuts for TaskItem.
The TaskItem extension has empty keyboard shortcuts configuration. Consider implementing keyboard shortcuts for better user experience, such as toggling task completion status.
addKeyboardShortcuts() {
- return {};
+ return {
+ 'Mod-Enter': () => this.editor.commands.toggleTaskCheck(),
+ 'Mod-Space': () => this.editor.commands.toggleTaskCheck(),
+ };
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TaskItem.extend({ | |
| parseHTML() { | |
| return []; | |
| }, | |
| addInputRules() { | |
| return []; | |
| }, | |
| addKeyboardShortcuts() { | |
| return {}; | |
| }, | |
| }).configure({ | |
| HTMLAttributes: { | |
| class: "relative", | |
| }, | |
| nested: true, | |
| }), | |
| TaskItem.extend({ | |
| parseHTML() { | |
| return []; | |
| }, | |
| addInputRules() { | |
| return []; | |
| }, | |
| addKeyboardShortcuts() { | |
| return { | |
| 'Mod-Enter': () => this.editor.commands.toggleTaskCheck(), | |
| 'Mod-Space': () => this.editor.commands.toggleTaskCheck(), | |
| }; | |
| }, | |
| }).configure({ | |
| HTMLAttributes: { | |
| class: "relative", | |
| }, | |
| nested: true, | |
| }), |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/editor/src/core/extensions/read-only-extensions.tsx (1)
108-135: Review task list configuration.The TaskList and TaskItem configurations have similar restrictions:
- Empty
parseHTMLmight affect existing task lists- Empty
addInputRulesremoves standard shortcuts- Empty
addKeyboardShortcutsin TaskItem might impact user experienceConsider maintaining some keyboard shortcuts for better accessibility, even in read-only mode.
TaskItem.extend({ parseHTML() { - return []; + return [ + { + tag: 'li[data-type="taskItem"]', + priority: 51, + }, + ]; }, addInputRules() { return []; }, addKeyboardShortcuts() { - return {}; + return { + 'Mod-Enter': () => true, // Allow task completion in read-only mode + }; }, }).configure({ HTMLAttributes: { class: "relative", }, nested: true, }),packages/editor/src/core/extensions/extensions.tsx (1)
86-121: Consider adding input rules for list extensions.The list extensions (BulletList, OrderedList, ListItem) have empty parseHTML and addInputRules methods. Consider adding input rules to support markdown-style list creation (e.g.,
-for bullet lists,1.for ordered lists).packages/editor/src/core/helpers/editor-commands.ts (1)
64-114: Enhance error handling in flat list commands.Consider adding try-catch blocks to handle potential errors during list creation, similar to the error handling in toggleCodeBlock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/editor/package.json(2 hunks)packages/editor/src/core/components/menus/menu-items.ts(4 hunks)packages/editor/src/core/extensions/extensions.tsx(5 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(4 hunks)packages/editor/src/core/helpers/editor-commands.ts(3 hunks)packages/editor/src/core/hooks/use-editor.ts(2 hunks)packages/editor/src/core/types/editor.ts(1 hunks)packages/editor/src/index.ts(1 hunks)packages/editor/src/styles/editor.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/editor/src/core/types/editor.ts
- packages/editor/src/index.ts
- packages/editor/package.json
🔇 Additional comments (13)
packages/editor/src/core/hooks/use-editor.ts (3)
11-11: LGTM! Import aligns with flat list migration feature.The import is properly organized within the flat-list extension directory.
110-111: LGTM! Clean state management for migration tracking.The boolean state effectively prevents unnecessary re-migrations.
112-139: Consider optimizing the migration effect.The effect might trigger unnecessarily due to frequent JSON changes. Consider these improvements:
- Add debouncing to prevent rapid re-runs
- Memoize the JSON comparison to avoid unnecessary migrations
+ import { useMemo, useCallback } from 'react'; + import debounce from 'lodash/debounce'; useEffect(() => { + const debouncedMigration = debounce(() => { if (editor && (!hasMigrated || editor.isActive("listItem") || editor.isActive("taskItem"))) { - const newJSON = migrateDocJSON(editor.getJSON() as any); + const currentJSON = editor.getJSON(); + const newJSON = useMemo(() => migrateDocJSON(currentJSON as any), [currentJSON]); // ... rest of the migration logic } + }, 300); + + debouncedMigration(); + return () => debouncedMigration.cancel(); }, [editor?.getJSON(), editor?.isActive("listItem"), editor?.isActive("taskItem"), hasMigrated]);Verify race conditions during migration.
The migration process might conflict with other editor operations. Ensure that:
- No other transactions are in progress during migration
- The editor state hasn't changed between getting JSON and applying migration
✅ Verification successful
Migration effect implementation is safe from race conditions
The transaction handling follows ProseMirror's synchronous state update pattern, and the migration is properly isolated from other editor operations. The suggested optimizations in the original review remain valid good-to-have improvements, but there are no race condition concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other places modifying editor state rg "editor\.state\.tr|transaction.*dispatch" -A 3Length of output: 2876
packages/editor/src/core/extensions/read-only-extensions.tsx (4)
2-5: LGTM! The new imports support flat list functionality.The added imports for list-related extensions align well with the PR objective of introducing flat lists.
Also applies to: 30-30
51-53: LGTM! Correctly disabled default list extensions.Disabling the default list extensions is necessary to prevent conflicts with the new flat list implementation.
72-107: Review the restrictive list configuration.The empty
parseHTMLandaddInputRulesmethods for BulletList, OrderedList, and ListItem might be too restrictive:
- Empty
parseHTMLmight prevent proper rendering of existing content- Empty
addInputRulesremoves standard input shortcutsConsider keeping some parsing rules for better compatibility with existing content.
71-71: Verify FlatListExtension configuration.The FlatListExtension is added without configuration. Please verify if it requires any specific configuration for read-only mode.
✅ Verification successful
FlatListExtension is correctly configured for read-only mode
The extension is a structural node that handles list functionality and doesn't require additional read-only configuration. It's properly integrated into both regular and read-only modes with all necessary components for list rendering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FlatListExtension implementation to verify its default configuration ast-grep --pattern 'export const FlatListExtension = Extension.create({ $$$ })'Length of output: 83
Script:
#!/bin/bash # Search for any mentions of FlatListExtension rg "FlatListExtension" -A 2 -B 2 # Search for imports containing FlatList rg "import.*FlatList.*from" # Search for any flat list related patterns rg -i "flatlist|flat.*list" -g "*.{ts,tsx,js,jsx}"Length of output: 12012
packages/editor/src/core/extensions/extensions.tsx (3)
61-63: LGTM: StarterKit configuration changes.The built-in list extensions are correctly disabled to prevent conflicts with the new flat list implementation. The dropcursor configuration is also properly updated.
Also applies to: 78-81
84-85: LGTM: Core extensions added.The DropCursor and FlatList extensions are properly integrated into the core extensions list.
134-149: Add keyboard shortcuts for task items.The TaskItem extension has empty keyboard shortcuts. Consider implementing shortcuts for better user experience.
addKeyboardShortcuts() { - return {}; + return { + 'Mod-Enter': () => this.editor.commands.toggleTaskCheck(), + 'Mod-Space': () => this.editor.commands.toggleTaskCheck(), + }; },packages/editor/src/core/components/menus/menu-items.ts (2)
158-159: LGTM: List menu items updated.The list menu items (BulletListItem, NumberedListItem, TodoListItem) are properly updated to use the new flat list commands and consistent isActive checks.
Also applies to: 166-167, 174-175
179-185: LGTM: ToggleListItem implementation.The new ToggleListItem follows the established pattern and integrates well with the existing menu items.
packages/editor/src/styles/editor.css (1)
411-411: LGTM: Simplified paragraph padding.The padding rules for paragraphs have been simplified to use a consistent padding-bottom of 4px, which improves maintainability.
| } catch (error) { | ||
| console.error("Error during migration:", error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for migration failures.
Current error handling only logs to console. Consider:
- Implementing a rollback mechanism
- Notifying users of migration failures
- Adding telemetry for migration errors
} catch (error) {
console.error("Error during migration:", error);
+ // Rollback to previous state
+ editor.commands.setContent(editor.getHTML());
+ // Notify user
+ editor.storage.notifications?.show({
+ type: 'error',
+ message: 'Failed to migrate list format. Please try again or contact support.'
+ });
+ // Report error
+ editor.storage.telemetry?.captureException(error);
}Committable suggestion skipped: line range outside the PR's diff.
| export const toggleFlatTaskList = (editor: Editor, range?: Range) => { | ||
| if (range) | ||
| editor | ||
| .chain() | ||
| .focus() | ||
| .deleteRange(range) | ||
| .createList({ | ||
| kind: "task", | ||
| }) | ||
| .run(); | ||
| else editor.chain().focus().createList({ kind: "task", collapsed: false }); | ||
| }; |
There was a problem hiding this comment.
Fix missing run() call in toggleFlatTaskList.
The else branch is missing the run() call, which will prevent the command from executing.
- else editor.chain().focus().createList({ kind: "task", collapsed: false });
+ else editor.chain().focus().createList({ kind: "task", collapsed: false }).run();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const toggleFlatTaskList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "task", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "task", collapsed: false }); | |
| }; | |
| export const toggleFlatTaskList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "task", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "task", collapsed: false }).run(); | |
| }; |
| export const toggleFlatBulletList = (editor: Editor, range?: Range) => { | ||
| if (range) | ||
| editor | ||
| .chain() | ||
| .focus() | ||
| .deleteRange(range) | ||
| .createList({ | ||
| kind: "bullet", | ||
| }) | ||
| .run(); | ||
| else editor.chain().focus().createList({ kind: "bullet", collapsed: false }); | ||
| }; |
There was a problem hiding this comment.
Fix missing run() call in toggleFlatBulletList.
The else branch is missing the run() call, which will prevent the command from executing.
- else editor.chain().focus().createList({ kind: "bullet", collapsed: false });
+ else editor.chain().focus().createList({ kind: "bullet", collapsed: false }).run();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const toggleFlatBulletList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "bullet", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "bullet", collapsed: false }); | |
| }; | |
| export const toggleFlatBulletList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "bullet", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "bullet", collapsed: false }).run(); | |
| }; |
| export const toggleFlatToggleList = (editor: Editor, range?: Range) => { | ||
| if (range) | ||
| editor | ||
| .chain() | ||
| .focus() | ||
| .deleteRange(range) | ||
| .createList({ | ||
| kind: "toggle", | ||
| }) | ||
| .run(); | ||
| else editor.chain().focus().createList({ kind: "toggle", collapsed: false }); | ||
| }; |
There was a problem hiding this comment.
Fix missing run() call in toggleFlatToggleList.
The else branch is missing the run() call, which will prevent the command from executing.
- else editor.chain().focus().createList({ kind: "toggle", collapsed: false });
+ else editor.chain().focus().createList({ kind: "toggle", collapsed: false }).run();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const toggleFlatToggleList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "toggle", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "toggle", collapsed: false }); | |
| }; | |
| export const toggleFlatToggleList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "toggle", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "toggle", collapsed: false }).run(); | |
| }; |
| export const toggleFlatOrderedList = (editor: Editor, range?: Range) => { | ||
| if (range) | ||
| editor | ||
| .chain() | ||
| .focus() | ||
| .deleteRange(range) | ||
| .createList({ | ||
| kind: "ordered", | ||
| }) | ||
| .run(); | ||
| else editor.chain().focus().createList({ kind: "ordered", collapsed: false }); | ||
| }; |
There was a problem hiding this comment.
Fix missing run() call in toggleFlatOrderedList.
The else branch is missing the run() call, which will prevent the command from executing.
- else editor.chain().focus().createList({ kind: "ordered", collapsed: false });
+ else editor.chain().focus().createList({ kind: "ordered", collapsed: false }).run();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const toggleFlatOrderedList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "ordered", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "ordered", collapsed: false }); | |
| }; | |
| export const toggleFlatOrderedList = (editor: Editor, range?: Range) => { | |
| if (range) | |
| editor | |
| .chain() | |
| .focus() | |
| .deleteRange(range) | |
| .createList({ | |
| kind: "ordered", | |
| }) | |
| .run(); | |
| else editor.chain().focus().createList({ kind: "ordered", collapsed: false }).run(); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/editor/src/core/helpers/editor-commands.ts (4)
64-71:⚠️ Potential issueFix missing run() call in toggleFlatOrderedList.
The command chain is incomplete without the run() call.
export const toggleFlatOrderedList = (editor: Editor, range?: Range) => { if (range) editor.chain().focus().deleteRange(range).createList({ kind: "ordered", collapsed: false, - }); + }).run(); - else editor.chain().focus().createList({ kind: "ordered", collapsed: false }); + else editor.chain().focus().createList({ kind: "ordered", collapsed: false }).run(); };
73-80:⚠️ Potential issueFix missing run() call in toggleFlatBulletList.
The command chain is incomplete without the run() call.
export const toggleFlatBulletList = (editor: Editor, range?: Range) => { if (range) editor.chain().focus().deleteRange(range).createList({ kind: "bullet", collapsed: false, - }); + }).run(); - else editor.chain().focus().createList({ kind: "bullet", collapsed: false }); + else editor.chain().focus().createList({ kind: "bullet", collapsed: false }).run(); };
82-89:⚠️ Potential issueFix missing run() call in toggleFlatTaskList.
The command chain is incomplete without the run() call.
export const toggleFlatTaskList = (editor: Editor, range?: Range) => { if (range) editor.chain().focus().deleteRange(range).createList({ kind: "task", collapsed: false, - }); + }).run(); - else editor.chain().focus().createList({ kind: "task", collapsed: false }); + else editor.chain().focus().createList({ kind: "task", collapsed: false }).run(); };
91-98:⚠️ Potential issueFix missing run() call in toggleFlatToggleList.
The command chain is incomplete without the run() call.
export const toggleFlatToggleList = (editor: Editor, range?: Range) => { if (range) editor.chain().focus().deleteRange(range).createList({ kind: "toggle", collapsed: false, - }); + }).run(); - else editor.chain().focus().createList({ kind: "toggle", collapsed: false }); + else editor.chain().focus().createList({ kind: "toggle", collapsed: false }).run(); };
🧹 Nitpick comments (19)
packages/editor/src/core/extensions/code/index.tsx (2)
19-23: Consider returning false when not incodeBlock
While this logic works for code blocks, if the user presses "Enter" outside a code block, the current function returnsundefined. Returningfalseexplicitly in the else case can improve clarity and consistency across your keyboard shortcut handlers.Enter: ({ editor }) => { if (editor.isActive("codeBlock")) { return editor.commands.newlineInCode(); } + return false; },
94-178: Potential duplication betweenTabandShift-Tablogic
TheShift-Tabhandler essentially replicates much of the logic in theTabhandler but in reverse. To improve maintainability, you could abstract the repeating logic (e.g., finding start and end of lines, building arrays of lines, managing offset calculations) into shared helper functions. This will reduce duplication and make future changes to indentation behavior simpler.+// Example excerpt of a shared function to find line boundaries for Tab or Shift-Tab: +function getSelectedLinesBounds(doc, from, to) { + // logic to find startPos and endPos + return { startPos, endPos }; +}packages/editor/src/core/extensions/mentions/extension.tsx (1)
28-37: Consider improving robustness and maintainability.A few suggestions to enhance the implementation:
- Add validation for the node structure
- Make the mention format configurable through options
- Add unit tests for the serialization logic
Here's a more robust implementation:
addStorage() { return { markdown: { serialize(state: MarkdownSerializerState, node: NodeType) { + // Validate node type + if (!node.type.spec.attrs?.[EMentionComponentAttributeNames.ENTITY_NAME]) { + console.warn('Invalid mention node structure'); + return; + } + const label = node.attrs[EMentionComponentAttributeNames.ENTITY_NAME] ?? "user_mention"; - state.write(`@${label}`); + // Use the mention format from options or fall back to default + const format = this.options.mentionFormat ?? '@{label}'; + const escapedLabel = label.replace(/([\\`*_{}[\]()#+\-.!])/g, '\\$1'); + state.write(format.replace('{label}', escapedLabel)); }, }, }; }packages/editor/src/core/extensions/flat-list/core/commands/split-list.ts (2)
25-29: Consider whether to preserve other relevant list attributes.Currently,
deriveListAttributesonly carries over the [kind] property. If future requirements need to preserve attributes likecollapsedorchecked, they must be explicitly handled here.
111-161: Avoid inserting null items into thetypesAfterarray.Including
nullin thetypesAfterarray may cause unexpected behavior or errors in certain ProseMirror APIs. IfnextTypeis missing, consider constructingtypesAfterwithout adding a null entry to avoid potential edge cases.You can apply this change as follows:
- const typesAfter = [{ type: listType, attrs: newAttrs }, nextType ? { type: nextType } : null]; + const typesAfter = nextType + ? [{ type: listType, attrs: newAttrs }, { type: nextType }] + : [{ type: listType, attrs: newAttrs }];packages/editor/src/core/extensions/code/without-props.tsx (1)
Line range hint
9-13: Document the changes in code block behavior.Since this is a significant change in code block interaction, consider:
- Updating documentation to reflect the new behavior
- Adding a comment explaining why keyboard shortcuts were removed
- Mentioning any alternative ways to achieve the previous functionality
+// Note: Custom keyboard shortcuts for Tab and Arrow keys were removed as part of the flat lists implementation. +// These actions now use the default CodeBlockLowlight behavior. export const CustomCodeBlockExtensionWithoutProps = CodeBlockLowlight.configure({ lowlight, defaultLanguage: "plaintext", exitOnTripleEnter: false, });packages/editor/src/core/extensions/table/table/utilities/insert-line-above-table-action.ts (1)
7-7: LGTM! Consider adding documentation about list interaction.The change maintains symmetry with
insertLineBelowTableActionand helps prevent conflicts between table and list behaviors.Consider adding a comment explaining why list state is checked:
// Check if the current selection or the closest node is a table + // Also prevent line insertion when inside a list to avoid conflicts with list behavior if (!editor.isActive("table") || editor.isActive("list")) return false;packages/editor/src/styles/lists.css (2)
25-91: Consider optimizing deeply nested selectors.While the hierarchical ordered list markers are well implemented, the deep nesting of selectors (up to 9 levels) might impact CSS performance. Consider using a more efficient approach.
&[data-list-kind="ordered"] { list-style: none; &::before { content: counter(prosemirror-flat-list-counter, decimal) "."; } - /* Second level - lower-alpha */ - & > .list-content > & { - &::before { - content: counter(prosemirror-flat-list-counter, lower-alpha) "."; - } - /* Third level - lower-roman */ - & > .list-content > & { - &::before { - content: counter(prosemirror-flat-list-counter, lower-roman) "."; - } - /* Start repeat pattern... */ + /* Use attribute selectors for nesting levels */ + &[data-level="1"]::before { + content: counter(prosemirror-flat-list-counter, decimal) "."; + } + &[data-level="2"]::before { + content: counter(prosemirror-flat-list-counter, lower-alpha) "."; + } + &[data-level="3"]::before { + content: counter(prosemirror-flat-list-counter, lower-roman) "."; + }
93-140: Consider optimizing bullet list selectors similarly.The bullet list implementation has the same deep nesting issue as the ordered list.
&[data-list-kind="bullet"] { list-style: none; - &::before { - content: "•"; - } - /* Second level - circle */ - & > .list-content > & { - &::before { - content: "⚬"; - } - /* Third level - square... */ + &[data-level="1"]::before { + content: "•"; + } + &[data-level="2"]::before { + content: "⚬"; + } + &[data-level="3"]::before { + content: "▪"; + }packages/editor/src/core/extensions/flat-list/list-extension.ts (3)
34-77: Add JSDoc documentation for the extension and its commands.Consider adding JSDoc documentation to describe the extension's purpose, parameters, and behavior. This will improve maintainability and help other developers understand the API.
Example:
+/** + * FlatListExtension provides enhanced list functionality with support for + * indentation, dedenting, and splitting operations. + */ export const FlatListExtension = Node.create({
54-57: Add error handling in command implementations.The commands should handle potential error cases and provide meaningful feedback.
Example for createList:
createList: (attrs: ListAttributes) => ({ state, view }) => { + try { const wrapInList = createWrapInListCommand<ListAttributes>(attrs); - return wrapInList(state, view.dispatch); + const result = wrapInList(state, view.dispatch); + if (!result) { + console.warn('Failed to create list with attrs:', attrs); + } + return result; + } catch (error) { + console.error('Error creating list:', error); + return false; + } },Also applies to: 60-63, 66-69, 72-75
105-105: Use a more descriptive variable name.Replace 'ans' with a more meaningful name like 'splitResult' or 'splitSuccess'.
- const ans = splitList(editor.state, editor.view.dispatch); - return ans; + const splitSuccess = splitList(editor.state, editor.view.dispatch); + return splitSuccess;packages/editor/src/core/extensions/drop-cursor.ts (4)
267-270: Use optional chaining for better null safety.Following the static analysis hint, improve null safety using optional chaining.
- const node = pos.inside >= 0 && this.editorView.state.doc.nodeAt(pos.inside); - const disableDropCursor = node && node.type.spec.disableDropCursor; + const node = pos.inside >= 0 ? this.editorView.state.doc.nodeAt(pos.inside) : null; + const disableDropCursor = node?.type.spec.disableDropCursor;🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
225-225: Extract magic numbers into named constants.The -20 pixel translation value should be defined as a named constant for better maintainability.
+const FLAT_LIST_CURSOR_OFFSET = -20; // ... - this.element.style.transform = isBetweenFlatList ? `translateX(${-20}px` : `translateX(0px)`; + this.element.style.transform = isBetweenFlatList ? `translateX(${FLAT_LIST_CURSOR_OFFSET}px` : `translateX(0px)`;
154-230: Consider breaking down the updateOverlay method.The method is quite long and handles multiple responsibilities. Consider extracting the positioning logic into separate methods.
Example structure:
private calculateRect($pos: ResolvedPos): Partial<DOMRect> { // Extract rect calculation logic } private calculateParentOffset(): { parentLeft: number; parentTop: number } { // Extract parent offset calculation logic } private updateElementStyles(rect: Partial<DOMRect>, offset: { parentLeft: number; parentTop: number }, isBetweenFlatList: boolean): void { // Extract style update logic }
334-436: Consider breaking down the complex rawIsBetweenFlatListsFn.The function is quite complex and handles multiple responsibilities. Consider extracting some logic into helper functions for better maintainability.
Example structure:
function handleToggleOrTaskList(currentFlatList: Element, state: State): void { // Extract toggle/task list logic } function handleNestedLists(currentFlatList: Element, state: State, getPositionFromElement: Function): void { // Extract nested list logic } function handleSiblingLists(currentFlatList: Element, state: State, getPositionFromElement: Function): void { // Extract sibling list logic }packages/editor/src/core/extensions/flat-list/core/commands/join-list-up.ts (3)
8-14: Enhance documentation with examples.The JSDoc comment clearly describes the functionality but would benefit from concrete examples showing the before/after states of the list structure in different scenarios.
Add examples like:
/** * ... * @example * // Example 1: Cursor at start of first child * Before: * - First item * - Nested item <-- cursor here * * After: * - First item * - Nested item * * // Example 2: Cursor at start of last child * ... */
30-61: Add error handling for failed transformations.The transformation logic lacks error handling for cases where operations might fail (e.g., invalid positions, content constraints).
Consider wrapping the transformation in a try-catch and handling potential errors:
if (dispatch) { + try { const tr = state.tr; // ... existing transformation code ... dispatch(tr); + } catch (error) { + console.warn('Failed to join list up:', error); + return false; + } }
90-105: Add return types and error handling to helper functions.The helper functions would benefit from explicit return type annotations and error handling for failed lift operations.
Consider these improvements:
-function liftListContent(state: EditorState, dispatch: (tr: Transaction) => void, $cursor: ResolvedPos) { +function liftListContent(state: EditorState, dispatch: (tr: Transaction) => void, $cursor: ResolvedPos): boolean { const tr = state.tr; const listDepth = $cursor.depth - 1; const range = new NodeRange($cursor, tr.doc.resolve($cursor.end(listDepth)), listDepth); if (safeLift(tr, range)) { dispatch(tr); + return true; } + return false; } -function liftParent(state: EditorState, dispatch: (tr: Transaction) => void, $cursor: ResolvedPos) { +function liftParent(state: EditorState, dispatch: (tr: Transaction) => void, $cursor: ResolvedPos): boolean { const tr = state.tr; const range = $cursor.blockRange(); if (range && safeLift(tr, range)) { dispatch(tr); + return true; } + return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/editor/src/core/extensions/code/index.tsx(2 hunks)packages/editor/src/core/extensions/code/without-props.tsx(1 hunks)packages/editor/src/core/extensions/drop-cursor.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/join-list-up.ts(1 hunks)packages/editor/src/core/extensions/flat-list/core/commands/split-list.ts(1 hunks)packages/editor/src/core/extensions/flat-list/list-extension.ts(1 hunks)packages/editor/src/core/extensions/mentions/extension.tsx(3 hunks)packages/editor/src/core/extensions/slash-commands/command-items-list.tsx(4 hunks)packages/editor/src/core/extensions/table/table/utilities/insert-line-above-table-action.ts(1 hunks)packages/editor/src/core/extensions/table/table/utilities/insert-line-below-table-action.ts(1 hunks)packages/editor/src/core/helpers/editor-commands.ts(3 hunks)packages/editor/src/core/helpers/insert-empty-paragraph-at-node-boundary.ts(2 hunks)packages/editor/src/styles/lists.css(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/editor/src/core/extensions/drop-cursor.ts
[error] 268-268: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 274-274: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (19)
packages/editor/src/core/extensions/code/index.tsx (2)
1-1: Successfully importedTextSelectionto support advanced cursor positioning
This addition looks good. ImportingTextSelectionis necessary for the more fine-grained selection adjustments introduced below.
28-73: Ensure correctness of multi-line tab insertion logic
The overall approach to insert tabs at the start of each selected line is valid. However, note these potential pitfalls and improvements:
- The offset adjustments look correct but can become tricky when lines partially overlap the selection start or end. Thoroughly test edge cases (e.g., partial line selections) to avoid misaligned cursor positions.
- Consider supporting user-configurable indentation characters (e.g., N spaces vs. a single tab character), as some developers might prefer spaces.
Would you like me to generate a test script for verifying correct behavior using a variety of multi-line selections?
packages/editor/src/core/extensions/mentions/extension.tsx (1)
2-3: LGTM! Necessary imports added for markdown serialization.The new imports from TipTap's ProseMirror packages are correctly added to support the markdown serialization functionality.
packages/editor/src/core/extensions/flat-list/core/commands/split-list.ts (3)
21-23: Solid use of chained commands with auto-fix list.The approach to wrap chained commands via
withAutoFixListlooks well-structured and ensures that any subsequent fixes on the list structure are applied automatically.
30-64: Confirm handling for lists with multiple child nodes.This command specifically targets a scenario where the list node has only one child. If multi-child splits in block node selections are expected, please verify or revise the logic to handle that case gracefully.
66-106: Flow for handling cursor at the first child vs. elsewhere is clear.The logic is well written—when the cursor is at the first child, it splits the list; otherwise, it carefully checks for empty parents before falling back to
enterWithoutLift. No changes needed here.packages/editor/src/core/extensions/code/without-props.tsx (1)
Line range hint
9-13: Verify the removal of keyboard shortcuts in code blocks.The removal of custom keyboard shortcuts (Tab, ArrowUp, ArrowDown) could impact the user experience when editing code blocks. Please ensure that:
- Basic code block functionality is preserved
- The default CodeBlockLowlight behavior is sufficient for your use case
✅ Verification successful
Keyboard shortcuts removal is part of the flat lists migration
The removal of local keyboard shortcuts from code blocks appears to be an intentional architectural change. The editor now handles keyboard interactions through:
- Global keymap management
- Flat list-specific keyboard handlers
- Default CodeBlockLowlight behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining keyboard shortcut handlers for code blocks # and verify if there are any related test files or documentation that need updating # Check for any other code block keyboard handlers echo "Checking for other code block keyboard handlers..." rg -l "addKeyboardShortcuts.*CodeBlock" # Check for related test files echo "Checking for related test files..." fd "code.*test|test.*code" -e ts -e tsx # Check for documentation mentioning code block keyboard shortcuts echo "Checking for documentation mentioning code block shortcuts..." rg -i "code.*(tab|arrow|keyboard)" -g "*.md"Length of output: 453
Script:
#!/bin/bash # Broaden search for code block related files and keyboard handling # Find all files related to code blocks echo "Finding code block related files..." fd -e ts -e tsx -e js -e jsx | rg -i "code" # Search for keyboard-related code in the editor echo "Searching for keyboard handling..." rg -l "keymap|keydown|keypress|keyboard" packages/editor # Look for any CodeBlock imports or usage echo "Searching for CodeBlock usage..." ast-grep --pattern 'import { $$ CodeBlock $$ } from'Length of output: 2016
packages/editor/src/core/extensions/table/table/utilities/insert-line-below-table-action.ts (1)
7-7: LGTM! Verify the interaction between tables and lists.The additional check for active list state helps prevent conflicts between table and list behaviors.
Let's verify that this change doesn't break existing table functionality:
✅ Verification successful
The list check is correctly implemented only where needed
The additional check for active list state is properly added to both line insertion actions (above and below), while other table-related features correctly maintain their original behavior as they serve different purposes (UI visibility, command availability, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other table-related commands that might need similar list state checks ast-grep --pattern 'editor.isActive("table")'Length of output: 687
Script:
#!/bin/bash # Get context for each table-active check to understand their purpose echo "=== extensions.tsx ===" rg -B 2 -A 2 'editor.isActive\("table"\)' packages/editor/src/core/extensions/extensions.tsx echo -e "\n=== root.tsx ===" rg -B 2 -A 2 'editor.isActive\("table"\)' packages/editor/src/core/extensions/slash-commands/root.tsx echo -e "\n=== editor-container.tsx ===" rg -B 2 -A 2 'editor.isActive\("table"\)' packages/editor/src/core/components/editors/editor-container.tsxLength of output: 938
packages/editor/src/core/helpers/insert-empty-paragraph-at-node-boundary.ts (3)
14-19: LGTM! Improved position handling.The simplified position handling using only
$frommakes the code cleaner while maintaining functionality.
25-34: LGTM! Enhanced boundary handling for upward insertion.Good addition of document boundary checks and special handling for document start.
51-74: LGTM! Enhanced boundary handling for downward insertion.Good addition of document boundary checks and special handling for document end.
packages/editor/src/core/extensions/slash-commands/command-items-list.tsx (2)
136-136: LGTM! Consistent update to flat list commands.Good replacement of old list commands with new flat list versions.
Also applies to: 145-145, 163-163
147-154: LGTM! Well-structured toggle list command.The new toggle list command follows the same pattern as other list commands and includes appropriate search terms.
packages/editor/src/core/helpers/editor-commands.ts (1)
173-174: LGTM! Simplified table insertion.Good removal of the clearNodes() call, making the table insertion more straightforward.
packages/editor/src/core/extensions/flat-list/list-extension.ts (2)
1-28: LGTM! Well-structured module declaration.The module declaration properly extends the Commands interface with clear type definitions for the new list commands.
123-149: LGTM! Efficient implementation with early exit.The function efficiently checks for list nodes in both directions using a single loop and includes an early exit optimization.
packages/editor/src/core/extensions/drop-cursor.ts (2)
438-476: LGTM! Well-implemented optimizations.The list level calculation and throttling implementations are clean, efficient, and well-documented.
40-41:⚠️ Potential issueAdd null check before destructuring rawIsBetweenFlatListsFn result.
The current implementation might throw if rawIsBetweenFlatListsFn returns null.
- const { isBetweenFlatLists, isHoveringOverListContent } = - rawIsBetweenFlatListsFn(event, tiptapEditorOptions.editor) || {}; + const result = rawIsBetweenFlatListsFn(event, tiptapEditorOptions.editor); + const { isBetweenFlatLists = false, isHoveringOverListContent = false } = result || {};Likely invalid or redundant comment.
packages/editor/src/core/extensions/flat-list/core/commands/join-list-up.ts (1)
15-88: Verify command integration and list type handling.Please ensure that:
- The command is properly registered in the editor's command system
- The implementation works correctly with all supported list types (bullet, ordered, task, toggle)
Let's verify the command registration and supported list types:
✅ Verification successful
Implementation verified - command is properly integrated
The command is correctly registered in the editor's command system and properly handles the flat list structure. The implementation is consistent with the extension's design of using a single list type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check command registration and list type handling # Find command registration echo "Checking command registration..." rg -l "joinListUp" packages/editor/src/core/extensions/flat-list # List all supported list types echo "Checking supported list types..." ast-grep --pattern 'type.name === "$_"' packages/editor/src/core/extensions/flat-listLength of output: 508
Script:
#!/bin/bash # Check command registration details and list types echo "Checking command registration in index.ts..." rg -A 5 "joinListUp" packages/editor/src/core/extensions/flat-list/core/index.ts echo -e "\nChecking keymap configuration..." rg -A 5 "joinListUp" packages/editor/src/core/extensions/flat-list/core/commands/keymap.ts echo -e "\nChecking list node types..." ast-grep --pattern 'createNode({ $$$ name: "$_", $$$ })' packages/editor/src/core/extensions/flat-listLength of output: 1554
Script:
#!/bin/bash # Find list node types and definitions echo "Searching for list node types..." rg -B2 -A2 "type\.name.*list" packages/editor/src/core/extensions/flat-list echo -e "\nSearching for node definitions..." rg -B2 -A2 "name:.*list" packages/editor/src/core/extensions/flat-listLength of output: 2250
| serialize(state: MarkdownSerializerState, node: NodeType) { | ||
| const label = node.attrs[EMentionComponentAttributeNames.ENTITY_NAME] ?? "user_mention"; | ||
| state.write(`@${label}`); | ||
| }, |
There was a problem hiding this comment.
Security: Sanitize user input before markdown serialization.
Direct interpolation of user input (label) into the markdown string could lead to markdown injection attacks. Consider escaping special markdown characters in the label.
Here's a safer implementation:
serialize(state: MarkdownSerializerState, node: NodeType) {
const label = node.attrs[EMentionComponentAttributeNames.ENTITY_NAME] ?? "user_mention";
- state.write(`@${label}`);
+ state.write('@' + label.replace(/([\\`*_{}[\]()#+\-.!])/g, '\\$1'));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| serialize(state: MarkdownSerializerState, node: NodeType) { | |
| const label = node.attrs[EMentionComponentAttributeNames.ENTITY_NAME] ?? "user_mention"; | |
| state.write(`@${label}`); | |
| }, | |
| serialize(state: MarkdownSerializerState, node: NodeType) { | |
| const label = node.attrs[EMentionComponentAttributeNames.ENTITY_NAME] ?? "user_mention"; | |
| state.write('@' + label.replace(/([\\`*_{}[\]()#+\-.!])/g, '\\$1')); | |
| }, |
| nodeBefore?.type.name === "list" && | ||
| nodeBefore?.lastChild.isBlock && | ||
| !nodeBefore.lastChild.type.name.startsWith("paragraph") | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding node type checks.
The condition !nodeBefore.lastChild.type.name.startsWith("paragraph") is fragile and may break if node types are renamed or if custom node types are introduced.
Consider using a more robust type checking approach:
- !nodeBefore.lastChild.type.name.startsWith("paragraph")
+ !nodeBefore.lastChild.type.spec.group?.includes("paragraph")Committable suggestion skipped: line range outside the PR's diff.
Description
This PR adds flat lists to our schema, a migration script that aims to safely convert old lists to new ones and a ton of new improvements to lists
Detailed description
TODO
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Performance